On Sat, Jul 14, 2018 at 08:49:28AM +0000, A.s. Dong wrote: > > -----Original Message----- > > From: Oleksij Rempel [mailto:o.rempel@xxxxxxxxxxxxxx] > > Sent: Saturday, July 14, 2018 3:02 PM > > To: A.s. Dong <aisheng.dong@xxxxxxx> > > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>; Fabio Estevam > > <fabio.estevam@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Mark > > Rutland <mark.rutland@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; dl-linux- > > imx <linux-imx@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > > kernel@xxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit > > > > Hi, > > > > Beside, what is equivalent of name and family name is in your name? > > I know it is different in China, so I won't to avoid confusion with: > > "Hi $name," format :) > > > > Dong is my family name. Either Hi A.S or Hi Dong is fine to me. :) > > > On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote: > > > Hi Oleksij, > > > > > > > -----Original Message----- > > > > From: Oleksij Rempel [mailto:o.rempel@xxxxxxxxxxxxxx] > > > > Sent: Friday, June 15, 2018 5:51 PM > > > > To: Shawn Guo <shawnguo@xxxxxxxxxx>; Fabio Estevam > > > > <fabio.estevam@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Mark > > > > Rutland <mark.rutland@xxxxxxx>; A.s. Dong <aisheng.dong@xxxxxxx> > > > > Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; > > kernel@xxxxxxxxxxxxxx; > > > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > > > > linux- clk@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > > > > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging > > > > unit > > > > > > > > The Mailbox controller is able to send messages (up to 4 32 bit > > > > words) between the endpoints. > > > > > > > > > > This is not correct according to current implementation as we abstract > > > them into 4 virtual channels while each 'channel' can send only one word > > one time. > > > We probably need explain such limitation in commit message as well. > > > > > > I'm not strongly against this way. But it makes the controller lose > > > the HW capability to send up to 4 words. I'd just like to know a bit > > > history or reason why we decided to do that. Do we design it for specific > > users case for M4? > > > > no, it is R&D. > > > > Got it > > > > And are we assuming there will be no real users of multi words send > > requirement? > > > > no. In my experience, each imaginable Brainfuck configuration will actually > > happen some day in some design for $reasons. > > So, no assumptions, just currently working configuration of my R&D project. > > > > I'm fine with as it is currently. We don't have to address all possible > Issues in one time. > > > > > This driver was tested using the mailbox-test driver sending > > > > messages between the Cortex-A7 and the Cortex-M4. > > > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> > > > > --- > > > > drivers/mailbox/Kconfig | 6 + > > > > drivers/mailbox/Makefile | 2 + > > > > drivers/mailbox/imx-mailbox.c | 288 > > > > ++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 296 insertions(+) > > > > create mode 100644 drivers/mailbox/imx-mailbox.c > > > > > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index > > > > a2bb27446dce..e1d2738a2e4c 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 "iMX Mailbox" > > > > + depends on SOC_IMX7D || COMPILE_TEST > > > > > > Better change to ARCH_MXC as other platform does. > > > > ok > > > > > > + help > > > > + Mailbox implementation for iMX7D Messaging Unit (MU). > > > > > > Ditto > > > > ok > > > > > > + > > > > 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..e3f621cb1d30 > > > > --- /dev/null > > > > +++ b/drivers/mailbox/imx-mailbox.c > > > > @@ -0,0 +1,288 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel > > > > +<o.rempel@xxxxxxxxxxxxxx> */ > > > > + > > > > +#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 + (x)) > > > > +#define IMX_MU_xSR_RFn(x) BIT(24 + (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 + (x)) > > > > +/* Receive Interrupt Enable */ > > > > +#define IMX_MU_xCR_RIEn(x) BIT(24 + (x)) > > > > + > > > > +#define IMX_MU_MAX_CHANS 4u > > > > + > > > > +struct imx_mu_priv; > > > > + > > > > +struct imx_mu_cfg { > > > > + unsigned int chans; > > > > + void (*init_hw)(struct imx_mu_priv *priv); }; > > > > + > > > > +struct imx_mu_con_priv { > > > > + int irq; > > > > + unsigned int bidx; > > > > + unsigned int idx; > > > > +}; > > > > + > > > > +struct imx_mu_priv { > > > > + struct device *dev; > > > > + const struct imx_mu_cfg *dcfg; > > > > + void __iomem *base; > > > > + > > > > + struct mbox_controller mbox; > > > > + struct mbox_chan mbox_chans[IMX_MU_MAX_CHANS]; > > > > + > > > > + struct imx_mu_con_priv con_priv[IMX_MU_MAX_CHANS]; > > > > + struct clk *clk; > > > > +}; > > > > + > > > > +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, dat; > > > > + > > > > + val = imx_mu_read(priv, IMX_MU_xSR); > > > > + val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx); > > > > + if (!val) > > > > + return IRQ_NONE; > > > > + > > > > + if (val & IMX_MU_xSR_TEn(cp->bidx)) { > > > > + imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp- > > > > >bidx)); > > > > + mbox_chan_txdone(chan, 0); > > > > + } > > > > + > > > > + if (val & IMX_MU_xSR_RFn(cp->bidx)) { > > > > + 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->bidx))); } > > > > + > > > > +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->bidx), 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; > > > > + > > > > + ret = request_irq(cp->irq, imx_mu_isr, > > > > + IRQF_SHARED, "imx_mu_chan", chan); > > > > > > I guess no need to assign the irq for each cp as we have only one irq. > > > > all irq chip controller have one sink and number of source limited to > > imagination or amount of bits in a register. May be we will need some day to > > write a irqchip driver to make it work as chained irq controller. > > > > We do need write an irqchip driver later because MU still supports another > four general purpose interrupts which will be used by SCU firmware. > But I don't think it's necessary to abstract them for TX/RX interrutps. > This makes thing simple. You just gave me one more reason to keep current version :) > > So, I don't see any technical reason to not do it. Are you? > > > > > > + if (ret) { > > > > + dev_err(chan->mbox->dev, > > > > + "Unable to acquire IRQ %d\n", cp->irq); > > > > + return ret; > > > > + } > > > > + > > > > + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 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->bidx) | IMX_MU_xCR_RIEn(cp- > > > > >bidx)); > > > > + > > > > + free_irq(cp->irq, chan); > > > > +} > > > > + > > > > +static const struct mbox_chan_ops imx_mu_ops = { > > > > + .send_data = imx_mu_send_data, > > > > + .startup = imx_mu_startup, > > > > + .shutdown = imx_mu_shutdown, > > > > +}; > > > > + > > > > +static int imx_mu_probe(struct platform_device *pdev) { > > > > + struct device *dev = &pdev->dev; > > > > + struct resource *iomem; > > > > + struct imx_mu_priv *priv; > > > > + const struct imx_mu_cfg *dcfg; > > > > + unsigned int i, chans; > > > > + int irq, ret; > > > > + > > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > > > + if (!priv) > > > > + return -ENOMEM; > > > > + > > > > + dcfg = of_device_get_match_data(dev); > > > > + if (!dcfg) > > > > + return -EINVAL; > > > > + > > > > + priv->dcfg = dcfg; > > > > + 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 < 0 ? irq : -EINVAL; > > > > + > > > > + priv->clk = devm_clk_get(dev, NULL); > > > > + if (IS_ERR(priv->clk)) { > > > > + if (PTR_ERR(priv->clk) == -ENOENT) { > > > > + priv->clk = NULL; > > > > + } else { > > > > + dev_err(dev, "Failed to get clock\n"); > > > > > > I guess we may not need print it for DEFER_PROBE error case. > > > > ok. > > > > > > + return PTR_ERR(priv->clk); > > > > + } > > > > + } > > > > + > > > > + ret = clk_prepare_enable(priv->clk); > > > > + if (ret) { > > > > + dev_err(dev, "Failed to enable clock\n"); > > > > + return ret; > > > > + } > > > > + > > > > + chans = min(dcfg->chans, IMX_MU_MAX_CHANS); > > > > + /* Initialize channel identifiers */ > > > > + for (i = 0; i < chans; i++) { > > > > + struct imx_mu_con_priv *cp = &priv->con_priv[i]; > > > > + > > > > + cp->bidx = 3 - i; > > > > > > We may not need it if we improve the macro to calculate bidx by idx? > > > > Are all implementation of NXP MU have reversed bit order? > > AFAIK NXP MU library is used for all known platforms with MUs. > So I guess yes. > > > Will it fit good for one channel implementation? > > If you see the SCU MU patches I sent, you will see I also need > convert the channel index to channel mask. But here you're > using bidx where SCU MU does have. So have a common > channel index to mask macro may be good for both using. ok > > > > > > + cp->idx = i; > > > > + cp->irq = irq; > > > > + priv->mbox_chans[i].con_priv = cp; > > > > + } > > > > + > > > > + priv->mbox.dev = dev; > > > > + priv->mbox.ops = &imx_mu_ops; > > > > + priv->mbox.chans = priv->mbox_chans; > > > > + priv->mbox.num_chans = chans; > > > > + priv->mbox.txdone_irq = true; > > > > + > > > > + platform_set_drvdata(pdev, priv); > > > > + > > > > + if (priv->dcfg->init_hw) > > > > + priv->dcfg->init_hw(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 void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) { > > > > + /* Set default config */ > > > > + imx_mu_write(priv, 0, IMX_MU_xCR); > > > > > > This will reset both MU Side A and B. > > > So we may need make sure Side B is initialized after A? > > > > I assume it is implementation specific, as soon as it will be needed, we may > > introduce extra DT flag. No need to cover all possible cases if we don't have > > to. > > > > We shouldn't reset SCU side, but SCU MU is not using it. So I'm okay. > We just need to know the limitation later. Probably a note added here > is better. ok > > > > +} > > > > + > > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = { > > > > + .chans = IMX_MU_MAX_CHANS, > > > > + .init_hw = imx_mu_init_imx7d_a, > > > > +}; > > > > + > > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = { > > > > + .chans = IMX_MU_MAX_CHANS, > > > > +}; > > > > + > > > > +static const struct of_device_id imx_mu_dt_ids[] = { > > > > + { .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a }, > > > > + { .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b }, > > > > > > I'm not sure whether we already have the decision to use fsl,<soc>-mu > > > compatible String and use property to specify the mu side. > > > Can you double check if we can switch to that way? > > > > ok. > > > > > And would you update the binding doc for M4 support according to the > > > qxp mu one Which Is already signed by Rob's tag? > > > > ok. > > > > So, should I update my patch set including DT binding documentation prior to > > yours? > > > > I guess you can pick that patch and send with yours. Once your part is > reviewed ok (should be quick) then I can send the SCU part based on your > Patch series. Normally it is preferred to squash all history for newly created files. I'll take your patch as base with minimal changes and send some comments to Rob. > Finally, I'm glad that we meet an agreement now. As we're trying to > Speed up the mx8qxp support and targets to hit v4.19 kernel. > So hopefully you could help send the updated patch series soon. > Then I can follow up with my work. :) I'll try to finish it and resend new version at Monday. Ok? > Regards > Dong Aisheng > > > If yes, can you please contact Rob to avoid confusions. > > > > -- > > Pengutronix e.K. | | > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
