| Message ID | 1405233128-4799-1-git-send-email-mollie.wu@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mollie, On 13/07/14 07:32, Mollie Wu wrote: > Add driver for the proprietary Mailbox controller (f_mhu) in MB86S7x. And it looks like this is not Fujitsu proprietary MHU, it's exactly same IP on JUNO(ARM64 development platform from ARM [1]). I was not sure it's standard IP used on other SoCs too, I too wrote similar driver :(. Can you please confirm this by reading Peripheral ID(PID: 0xFD0, 0xFE0 - 0xFEC) and Component ID(COMPID: 0xFF0 - 0xFFC). Are they PID - 0x04 0x98 0xB0 0x1B 0x00 COMPID - 0x0D 0xF0 0x05 0xB1 If so we have do s/f_mhu/arm_mhu/g :) > It has three channels - LowPri-NonSecure, HighPri-NonSecure and Secure. > The MB86S7x communicates over the HighPri-NonSecure channel. > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com> > Signed-off-by: Mollie Wu <mollie.wu@linaro.org> > --- > drivers/mailbox/Kconfig | 7 ++ > drivers/mailbox/Makefile | 2 + > drivers/mailbox/f_mhu.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 236 insertions(+) > create mode 100644 drivers/mailbox/f_mhu.c > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > index c8b5c13..681aac2 100644 > --- a/drivers/mailbox/Kconfig > +++ b/drivers/mailbox/Kconfig > @@ -6,6 +6,13 @@ menuconfig MAILBOX > signals. Say Y if your platform supports hardware mailboxes. > > if MAILBOX > + > +config MBOX_F_MHU > + bool On Juno, there's nothing that prevents me from compiling this as module. > + depends on ARCH_MB86S7X Definitely not a requirement > + help > + Say Y here if you want to use the F_MHU IPCM support. > + Also it needs some description. > config PL320_MBOX > bool "ARM PL320 Mailbox" > depends on ARM_AMBA > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile > index 2fa343a..3707e93 100644 > --- a/drivers/mailbox/Makefile > +++ b/drivers/mailbox/Makefile > @@ -2,6 +2,8 @@ > > obj-$(CONFIG_MAILBOX) += mailbox.o > > +obj-$(CONFIG_MBOX_F_MHU) += f_mhu.o > + > obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o > > obj-$(CONFIG_OMAP_MBOX) += omap-mailbox.o > diff --git a/drivers/mailbox/f_mhu.c b/drivers/mailbox/f_mhu.c > new file mode 100644 > index 0000000..cf5d3cd > --- /dev/null > +++ b/drivers/mailbox/f_mhu.c > @@ -0,0 +1,227 @@ > +/* > + * Copyright (C) 2013-2014 Fujitsu Semiconductor Ltd. > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/spinlock.h> > +#include <linux/mutex.h> > +#include <linux/delay.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/clk.h> > +#include <linux/module.h> > +#include <linux/mailbox_controller.h> > +#include <linux/platform_device.h> > + > +#define INTR_STAT_OFS 0x0 > +#define INTR_SET_OFS 0x8 > +#define INTR_CLR_OFS 0x10 > + > +#define MHU_SCFG 0x400 > + Remove this.(secure access only register) > +struct mhu_link { > + unsigned irq; > + spinlock_t lock; /* channel regs */ > + void __iomem *tx_reg; > + void __iomem *rx_reg; > +}; > + > +struct f_mhu { > + void __iomem *base; > + struct clk *clk; > + struct mhu_link mlink[3]; > + struct mbox_chan chan[3]; > + struct mbox_controller mbox; > +}; > + > +static irqreturn_t mhu_rx_interrupt(int irq, void *p) > +{ > + struct mbox_chan *chan = (struct mbox_chan *)p; > + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; You don't need explicit cast from void pointers > + u32 val; > + > + pr_debug("%s:%d\n", __func__, __LINE__); Please remove all these debug prints. > + /* See NOTE_RX_DONE */ > + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); > + mbox_chan_received_data(chan, (void *)val); > + > + /* > + * It is agreed with the remote firmware that the receiver > + * will clear the STAT register indicating it is ready to > + * receive next data - NOTE_RX_DONE > + */ This note could be added as how this mailbox works in general and it's not just Rx right ? Even Tx done is based on this logic. Basically the logic is valid on both directions. > + writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS); > + > + return IRQ_HANDLED; > +} > + > +static bool mhu_last_tx_done(struct mbox_chan *chan) > +{ > + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; > + unsigned long flags; > + u32 val; > + > + pr_debug("%s:%d\n", __func__, __LINE__); > + spin_lock_irqsave(&mlink->lock, flags); Why do we need this extra locks here ? mailbox core maintains per channel lock and uses it for at-least send_data IIRC. And this function is just reading status do we really need the lock ? I must be missing something here else IMO we can get rid of this extra locks in here. > + /* See NOTE_RX_DONE */ > + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); > + spin_unlock_irqrestore(&mlink->lock, flags); > + > + return (val == 0); > +} > + > +static int mhu_send_data(struct mbox_chan *chan, void *data) > +{ > + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; > + unsigned long flags; > + > + pr_debug("%s:%d\n", __func__, __LINE__); > + if (!mhu_last_tx_done(chan)) { > + pr_err("%s:%d Shouldn't have seen the day!\n", > + __func__, __LINE__); > + return -EBUSY; > + } > + > + spin_lock_irqsave(&mlink->lock, flags); > + writel_relaxed((u32)data, mlink->tx_reg + INTR_SET_OFS); > + spin_unlock_irqrestore(&mlink->lock, flags); > + > + return 0; > +} > + > +static int mhu_startup(struct mbox_chan *chan) > +{ > + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; > + unsigned long flags; > + u32 val; > + int ret; > + > + pr_debug("%s:%d\n", __func__, __LINE__); > + spin_lock_irqsave(&mlink->lock, flags); > + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); > + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); > + spin_unlock_irqrestore(&mlink->lock, flags); > + > + ret = request_irq(mlink->irq, mhu_rx_interrupt, > + IRQF_SHARED, "mhu_link", chan); Just a thought: Can this be threaded_irq instead ? Can move request_irq to probe instead esp. if threaded_irq ? That provides some flexibility to client's rx_callback. > + if (unlikely(ret)) { > + pr_err("Unable to aquire IRQ\n"); > + return ret; > + } > + > + return 0; > +} > + > +static void mhu_shutdown(struct mbox_chan *chan) > +{ > + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; > + > + pr_debug("%s:%d\n", __func__, __LINE__); > + free_irq(mlink->irq, chan); > +} > + > +static struct mbox_chan_ops mhu_ops = { > + .send_data = mhu_send_data, > + .startup = mhu_startup, > + .shutdown = mhu_shutdown, > + .last_tx_done = mhu_last_tx_done, > +}; > + > +static int f_mhu_probe(struct platform_device *pdev) > +{ > + int i, err; > + struct f_mhu *mhu; > + struct resource *res; > + int mhu_reg[3] = {0x0, 0x20, 0x200}; Probably this gets simplified when you remove secure channel access ? > + > + /* Allocate memory for device */ > + mhu = kzalloc(sizeof(*mhu), GFP_KERNEL); > + if (!mhu) { > + dev_err(&pdev->dev, "failed to allocate memory.\n"); > + return -EBUSY; > + } > + > + mhu->clk = clk_get(&pdev->dev, "clk"); > + if (unlikely(IS_ERR(mhu->clk))) { > + dev_err(&pdev->dev, "unable to init clock\n"); Don't bail out if there's no clock specified in DT. Clock might not be a hard requirement. > + kfree(mhu); > + return -EINVAL; > + } > + clk_prepare_enable(mhu->clk); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mhu->base = ioremap(res->start, resource_size(res)); > + if (!mhu->base) { > + dev_err(&pdev->dev, "ioremap failed.\n"); > + kfree(mhu); > + return -EBUSY; > + } > + > + /* Let UnTrustedOS's access violations don't bother us */ > + writel_relaxed(0, mhu->base + MHU_SCFG); > + Please don't do this. It can't work in non-secure mode. The firmware running with secure access needs to configure this appropriately. I might be missing to see, is there a binding document for this mhu ? > + for (i = 0; i < 3; i++) { > + mhu->chan[i].con_priv = &mhu->mlink[i]; > + spin_lock_init(&mhu->mlink[i].lock); > + res = platform_get_resource(pdev, IORESOURCE_IRQ, i); > + mhu->mlink[i].irq = res->start; > + mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i]; > + mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + 0x100; > + } > + > + mhu->mbox.dev = &pdev->dev; > + mhu->mbox.chans = &mhu->chan[0]; > + mhu->mbox.num_chans = 3; Change this to 2, we shouldn't expose secular channel here as Linux can't access that anyway. > + mhu->mbox.ops = &mhu_ops; > + mhu->mbox.txdone_irq = false; > + mhu->mbox.txdone_poll = true; > + mhu->mbox.txpoll_period = 10; > + > + platform_set_drvdata(pdev, mhu); > + > + err = mbox_controller_register(&mhu->mbox); > + if (err) { > + dev_err(&pdev->dev, "Failed to register mailboxes %d\n", err); > + iounmap(mhu->base); > + kfree(mhu); > + } else { > + dev_info(&pdev->dev, "Fujitsu MHU Mailbox registered\n"); > + } > + > + return 0; > +} > + Also to be module you need add remove. > +static const struct of_device_id f_mhu_dt_ids[] = { > + { .compatible = "fujitsu,mhu" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, f_mhu_dt_ids); > + > +static struct platform_driver f_mhu_driver = { > + .driver = { > + .name = "f_mhu", > + .owner = THIS_MODULE, > + .of_match_table = f_mhu_dt_ids, > + }, > + .probe = f_mhu_probe, > +}; > + > +static int __init f_mhu_init(void) > +{ > + return platform_driver_register(&f_mhu_driver); > +} > +module_init(f_mhu_init); This can be module_platform_driver instead. Regards, Sudeep [1] http://www.arm.com/products/tools/development-boards/versatile-express/juno-arm-development-platform.php
On 16 July 2014 23:07, Sudeep Holla <sudeep.holla@arm.com> wrote: > Hi Mollie, > > > On 13/07/14 07:32, Mollie Wu wrote: >> >> Add driver for the proprietary Mailbox controller (f_mhu) in MB86S7x. > > > And it looks like this is not Fujitsu proprietary MHU, it's exactly same IP > on JUNO(ARM64 development platform from ARM [1]). I was not sure it's > standard > IP used on other SoCs too, I too wrote similar driver :(. > Same here :) > Can you please confirm this by reading Peripheral ID(PID: 0xFD0, 0xFE0 - > 0xFEC) and Component ID(COMPID: 0xFF0 - 0xFFC). Are they > > PID - 0x04 0x98 0xB0 0x1B 0x00 > COMPID - 0x0D 0xF0 0x05 0xB1 > > If so we have do s/f_mhu/arm_mhu/g :) > Yup, we have to. > >> It has three channels - LowPri-NonSecure, HighPri-NonSecure and Secure. >> The MB86S7x communicates over the HighPri-NonSecure channel. >> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> >> Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com> >> Signed-off-by: Mollie Wu <mollie.wu@linaro.org> >> --- >> drivers/mailbox/Kconfig | 7 ++ >> drivers/mailbox/Makefile | 2 + >> drivers/mailbox/f_mhu.c | 227 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 236 insertions(+) >> create mode 100644 drivers/mailbox/f_mhu.c >> >> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig >> index c8b5c13..681aac2 100644 >> --- a/drivers/mailbox/Kconfig >> +++ b/drivers/mailbox/Kconfig >> @@ -6,6 +6,13 @@ menuconfig MAILBOX >> signals. Say Y if your platform supports hardware mailboxes. >> >> if MAILBOX >> + >> +config MBOX_F_MHU >> + bool > > > On Juno, there's nothing that prevents me from compiling this as module. > On S7x, even built-in is not early enough for our purposes. But yes, now that we have company it should be made tristate. >> + depends on ARCH_MB86S7X > > > Definitely not a requirement > It was, until we found each other ;) > >> + help >> + Say Y here if you want to use the F_MHU IPCM support. >> + > > > Also it needs some description. > OK >> +#define INTR_STAT_OFS 0x0 >> +#define INTR_SET_OFS 0x8 >> +#define INTR_CLR_OFS 0x10 >> + >> +#define MHU_SCFG 0x400 >> + > > > Remove this.(secure access only register) > See later. > >> +struct mhu_link { >> + unsigned irq; >> + spinlock_t lock; /* channel regs */ >> + void __iomem *tx_reg; >> + void __iomem *rx_reg; >> +}; >> + >> +struct f_mhu { >> + void __iomem *base; >> + struct clk *clk; >> + struct mhu_link mlink[3]; >> + struct mbox_chan chan[3]; >> + struct mbox_controller mbox; >> +}; >> + >> +static irqreturn_t mhu_rx_interrupt(int irq, void *p) >> +{ >> + struct mbox_chan *chan = (struct mbox_chan *)p; >> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; > > > You don't need explicit cast from void pointers > Yup > >> + u32 val; >> + >> + pr_debug("%s:%d\n", __func__, __LINE__); > > > Please remove all these debug prints. > These are as good as absent unless DEBUG is defined. > >> + /* See NOTE_RX_DONE */ >> + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); >> + mbox_chan_received_data(chan, (void *)val); >> + >> + /* >> + * It is agreed with the remote firmware that the receiver >> + * will clear the STAT register indicating it is ready to >> + * receive next data - NOTE_RX_DONE >> + */ > > This note could be added as how this mailbox works in general and > it's not just Rx right ? Even Tx done is based on this logic. > Basically the logic is valid on both directions. > Yes that is a protocol level agreement. Different f/w may behave differently. > >> + writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static bool mhu_last_tx_done(struct mbox_chan *chan) >> +{ >> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >> + unsigned long flags; >> + u32 val; >> + >> + pr_debug("%s:%d\n", __func__, __LINE__); >> + spin_lock_irqsave(&mlink->lock, flags); > > > Why do we need this extra locks here ? mailbox core maintains > per channel lock and uses it for at-least send_data IIRC. And this > function is just reading status do we really need the lock ? > > I must be missing something here else IMO we can get rid of this > extra locks in here. > Yeah, probably unnecessary. >> + /* See NOTE_RX_DONE */ >> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >> + spin_unlock_irqrestore(&mlink->lock, flags); >> + >> + return (val == 0); >> +} >> + >> +static int mhu_send_data(struct mbox_chan *chan, void *data) >> +{ >> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >> + unsigned long flags; >> + >> + pr_debug("%s:%d\n", __func__, __LINE__); >> + if (!mhu_last_tx_done(chan)) { >> + pr_err("%s:%d Shouldn't have seen the day!\n", >> + __func__, __LINE__); >> + return -EBUSY; >> + } >> + >> + spin_lock_irqsave(&mlink->lock, flags); >> + writel_relaxed((u32)data, mlink->tx_reg + INTR_SET_OFS); >> + spin_unlock_irqrestore(&mlink->lock, flags); >> + >> + return 0; >> +} >> + >> +static int mhu_startup(struct mbox_chan *chan) >> +{ >> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >> + unsigned long flags; >> + u32 val; >> + int ret; >> + >> + pr_debug("%s:%d\n", __func__, __LINE__); >> + spin_lock_irqsave(&mlink->lock, flags); >> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >> + spin_unlock_irqrestore(&mlink->lock, flags); >> + >> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >> + IRQF_SHARED, "mhu_link", chan); > > > Just a thought: Can this be threaded_irq instead ? > Can move request_irq to probe instead esp. if threaded_irq ? > That provides some flexibility to client's rx_callback. > This is controller driver, and can not know which client want rx_callback in hard-irq context and which in thread_fn context. Otherwise, request_irq() does evaluate to request_threaded_irq(), if thats what you mean. There might be use-cases like (diagnostic or other) data transfer over mailbox where we don't wanna increase latency, so we have to provide a rx_callback in hard-irq context. > >> + if (unlikely(ret)) { >> + pr_err("Unable to aquire IRQ\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static void mhu_shutdown(struct mbox_chan *chan) >> +{ >> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >> + >> + pr_debug("%s:%d\n", __func__, __LINE__); >> + free_irq(mlink->irq, chan); >> +} >> + >> +static struct mbox_chan_ops mhu_ops = { >> + .send_data = mhu_send_data, >> + .startup = mhu_startup, >> + .shutdown = mhu_shutdown, >> + .last_tx_done = mhu_last_tx_done, >> +}; >> + >> +static int f_mhu_probe(struct platform_device *pdev) >> +{ >> + int i, err; >> + struct f_mhu *mhu; >> + struct resource *res; >> + int mhu_reg[3] = {0x0, 0x20, 0x200}; > > > Probably this gets simplified when you remove secure channel access ? > we don't remove secure channel :) > >> + >> + /* Allocate memory for device */ >> + mhu = kzalloc(sizeof(*mhu), GFP_KERNEL); >> + if (!mhu) { >> + dev_err(&pdev->dev, "failed to allocate memory.\n"); >> + return -EBUSY; >> + } >> + >> + mhu->clk = clk_get(&pdev->dev, "clk"); >> + if (unlikely(IS_ERR(mhu->clk))) { >> + dev_err(&pdev->dev, "unable to init clock\n"); > > > Don't bail out if there's no clock specified in DT. Clock might not > be a hard requirement. > Hmm.. OK. > >> + kfree(mhu); >> + return -EINVAL; >> + } >> + clk_prepare_enable(mhu->clk); >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + mhu->base = ioremap(res->start, resource_size(res)); >> + if (!mhu->base) { >> + dev_err(&pdev->dev, "ioremap failed.\n"); >> + kfree(mhu); >> + return -EBUSY; >> + } >> + >> + /* Let UnTrustedOS's access violations don't bother us */ >> + writel_relaxed(0, mhu->base + MHU_SCFG); >> + > > > Please don't do this. It can't work in non-secure mode. The firmware running > with secure access needs to configure this appropriately. > ok.. pls see below. > I might be missing to see, is there a binding document for this mhu ? > Yeah we need one. > >> + for (i = 0; i < 3; i++) { >> + mhu->chan[i].con_priv = &mhu->mlink[i]; >> + spin_lock_init(&mhu->mlink[i].lock); >> + res = platform_get_resource(pdev, IORESOURCE_IRQ, i); >> + mhu->mlink[i].irq = res->start; >> + mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i]; >> + mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + 0x100; >> + } >> + >> + mhu->mbox.dev = &pdev->dev; >> + mhu->mbox.chans = &mhu->chan[0]; >> + mhu->mbox.num_chans = 3; > > > Change this to 2, we shouldn't expose secular channel here as Linux can't > access that anyway. > On the contrary, I think the device driver code should be able to manage every resource - secure or non-secure. If we remove secure channel option, what do we do on some other platform that needs it? And Linux may not always run in non-secure mode. So here we populate all channels and let the clients knows which channel to use via platform specific details - DT. Please note the driver doesn't touch any secure resource if client doesn't ask it to (except SCFG for now, which I think should have some S vs NS DT binding). > >> + mhu->mbox.ops = &mhu_ops; >> + mhu->mbox.txdone_irq = false; >> + mhu->mbox.txdone_poll = true; >> + mhu->mbox.txpoll_period = 10; >> + >> + platform_set_drvdata(pdev, mhu); >> + >> + err = mbox_controller_register(&mhu->mbox); >> + if (err) { >> + dev_err(&pdev->dev, "Failed to register mailboxes %d\n", >> err); >> + iounmap(mhu->base); >> + kfree(mhu); >> + } else { >> + dev_info(&pdev->dev, "Fujitsu MHU Mailbox registered\n"); >> + } >> + >> + return 0; >> +} >> + > > > Also to be module you need add remove. > Yup, OK. > >> +static const struct of_device_id f_mhu_dt_ids[] = { >> + { .compatible = "fujitsu,mhu" }, >> + { /* sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, f_mhu_dt_ids); >> + >> +static struct platform_driver f_mhu_driver = { >> + .driver = { >> + .name = "f_mhu", >> + .owner = THIS_MODULE, >> + .of_match_table = f_mhu_dt_ids, >> + }, >> + .probe = f_mhu_probe, >> +}; >> + >> +static int __init f_mhu_init(void) >> +{ >> + return platform_driver_register(&f_mhu_driver); >> +} >> +module_init(f_mhu_init); > > > This can be module_platform_driver instead. > OK Thanks -Jassi
On 17/07/14 07:25, Jassi Brar wrote: > On 16 July 2014 23:07, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Hi Mollie, >> >> >> On 13/07/14 07:32, Mollie Wu wrote: >>> >>> Add driver for the proprietary Mailbox controller (f_mhu) in MB86S7x. [...] >> >>> + u32 val; >>> + >>> + pr_debug("%s:%d\n", __func__, __LINE__); >> >> >> Please remove all these debug prints. >> > These are as good as absent unless DEBUG is defined. > Yes, but with mailbox framework, the controller driver(esp. this one)is almost like a shim layer. Instead of each driver adding these debugs, IMO it would be good to have these in the framework. >> >>> + /* See NOTE_RX_DONE */ >>> + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); >>> + mbox_chan_received_data(chan, (void *)val); >>> + >>> + /* >>> + * It is agreed with the remote firmware that the receiver >>> + * will clear the STAT register indicating it is ready to >>> + * receive next data - NOTE_RX_DONE >>> + */ >> >> This note could be added as how this mailbox works in general and >> it's not just Rx right ? Even Tx done is based on this logic. >> Basically the logic is valid on both directions. >> > Yes that is a protocol level agreement. Different f/w may behave differently. > Ok, I am not sure if that's entirely true because the MHU spec says it drives the signal using a 32-bit register, with all 32 bits logically ORed together. Usually only Rx signals are wired to interrupts and Tx needs to be polled but that's entirely implementation choice I believe. More over if it's protocol level agreement it should not belong here :) >>> +static int mhu_startup(struct mbox_chan *chan) >>> +{ >>> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >>> + unsigned long flags; >>> + u32 val; >>> + int ret; >>> + >>> + pr_debug("%s:%d\n", __func__, __LINE__); >>> + spin_lock_irqsave(&mlink->lock, flags); >>> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >>> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >>> + spin_unlock_irqrestore(&mlink->lock, flags); >>> + >>> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >>> + IRQF_SHARED, "mhu_link", chan); >> >> >> Just a thought: Can this be threaded_irq instead ? >> Can move request_irq to probe instead esp. if threaded_irq ? >> That provides some flexibility to client's rx_callback. >> > This is controller driver, and can not know which client want > rx_callback in hard-irq context and which in thread_fn context. > Otherwise, request_irq() does evaluate to request_threaded_irq(), if > thats what you mean. Agreed but on contrary since MHU involves external firmware(running on different processor) which might have it's own latency, I prefer threaded over hard irq. And yes request_irq does evaluate to request_threaded_irq but with thread_fn = NULL which is not what I want. > There might be use-cases like (diagnostic or other) data transfer > over mailbox where we don't wanna increase latency, so we have to > provide a rx_callback in hard-irq context. > OK >> >>> + for (i = 0; i < 3; i++) { >>> + mhu->chan[i].con_priv = &mhu->mlink[i]; >>> + spin_lock_init(&mhu->mlink[i].lock); >>> + res = platform_get_resource(pdev, IORESOURCE_IRQ, i); >>> + mhu->mlink[i].irq = res->start; >>> + mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i]; >>> + mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + 0x100; >>> + } >>> + >>> + mhu->mbox.dev = &pdev->dev; >>> + mhu->mbox.chans = &mhu->chan[0]; >>> + mhu->mbox.num_chans = 3; >> >> >> Change this to 2, we shouldn't expose secular channel here as Linux can't >> access that anyway. >> > On the contrary, I think the device driver code should be able to > manage every resource - secure or non-secure. If we remove secure > channel option, what do we do on some other platform that needs it? > And Linux may not always run in non-secure mode. In general secure accesses are avoided these days in Linux as we have no way to identify it. I agree there are few place where we do access. Even though I don't like you have secure channel access in Linux, you have valid reasons. In case you decide to support it, there is some restrictions in bit 31, though RAZ/WI you need to notify that to protocol if it tries to access it ? > So here we populate all channels and let the clients knows which > channel to use via platform specific details - DT. Please note the > driver doesn't touch any secure resource if client doesn't ask it to > (except SCFG for now, which I think should have some S vs NS DT > binding). > Not sure of this though. We need feedback from DT maintainers. Regards, Sudeep
On 17 July 2014 16:01, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 17/07/14 07:25, Jassi Brar wrote: >> >>> >>>> + u32 val; >>>> + >>>> + pr_debug("%s:%d\n", __func__, __LINE__); >>> >>> >>> >>> Please remove all these debug prints. >>> >> These are as good as absent unless DEBUG is defined. >> > > Yes, but with mailbox framework, the controller driver(esp. this one)is > almost like a shim layer. Instead of each driver adding these debugs, > IMO it would be good to have these in the framework. > OK >>> >>>> + /* See NOTE_RX_DONE */ >>>> + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); >>>> + mbox_chan_received_data(chan, (void *)val); >>>> + >>>> + /* >>>> + * It is agreed with the remote firmware that the receiver >>>> + * will clear the STAT register indicating it is ready to >>>> + * receive next data - NOTE_RX_DONE >>>> + */ >>> >>> >>> This note could be added as how this mailbox works in general and >>> it's not just Rx right ? Even Tx done is based on this logic. >>> Basically the logic is valid on both directions. >>> >> Yes that is a protocol level agreement. Different f/w may behave >> differently. >> > > Ok, I am not sure if that's entirely true because the MHU spec says it > drives > the signal using a 32-bit register, with all 32 bits logically ORed > together. > Usually only Rx signals are wired to interrupts and Tx needs to be polled > but that's entirely implementation choice I believe. > On my platform, _STAT register only carries the command code but actual data is exchanged via SharedMemory/SHM. Now we need to know when the sent data packet (in SHM) has been consumed by the remote - for which our protocol mandates the remote clear the TX_STAT register. And vice-versa for RX. Some other f/w may choose some other mechanism for TX-Done - say some ACK bit set or even some time bound guarantee. > More over if it's protocol level agreement it should not belong here :) > My f/w expects the RX_STAT cleared after reading data from SHM. Where do you think is the right place to clear RX_STAT? I have said many times in many threads... its the mailbox controller _and_ the remote f/w that should be seen as one entity. It may not be possible to write a controller driver that works with any remote firmware. > >>>> +static int mhu_startup(struct mbox_chan *chan) >>>> +{ >>>> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >>>> + unsigned long flags; >>>> + u32 val; >>>> + int ret; >>>> + >>>> + pr_debug("%s:%d\n", __func__, __LINE__); >>>> + spin_lock_irqsave(&mlink->lock, flags); >>>> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >>>> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >>>> + spin_unlock_irqrestore(&mlink->lock, flags); >>>> + >>>> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >>>> + IRQF_SHARED, "mhu_link", chan); >>> >>> >>> >>> Just a thought: Can this be threaded_irq instead ? >>> Can move request_irq to probe instead esp. if threaded_irq ? >>> That provides some flexibility to client's rx_callback. >>> >> This is controller driver, and can not know which client want >> rx_callback in hard-irq context and which in thread_fn context. >> Otherwise, request_irq() does evaluate to request_threaded_irq(), if >> thats what you mean. > > Agreed but on contrary since MHU involves external firmware(running > on different processor) which might have it's own latency, I prefer > threaded over hard irq. And yes request_irq does evaluate to > request_threaded_irq but with thread_fn = NULL which is not what I want. > If remote has its own latency, that does not mean we add some more :) >> There might be use-cases like (diagnostic or other) data transfer >> over mailbox where we don't wanna increase latency, so we have to >> provide a rx_callback in hard-irq context. >> > OK > Cool. > >>> >>>> + for (i = 0; i < 3; i++) { >>>> + mhu->chan[i].con_priv = &mhu->mlink[i]; >>>> + spin_lock_init(&mhu->mlink[i].lock); >>>> + res = platform_get_resource(pdev, IORESOURCE_IRQ, i); >>>> + mhu->mlink[i].irq = res->start; >>>> + mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i]; >>>> + mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + 0x100; >>>> + } >>>> + >>>> + mhu->mbox.dev = &pdev->dev; >>>> + mhu->mbox.chans = &mhu->chan[0]; >>>> + mhu->mbox.num_chans = 3; >>> >>> >>> >>> Change this to 2, we shouldn't expose secular channel here as Linux can't >>> access that anyway. >>> >> On the contrary, I think the device driver code should be able to >> manage every resource - secure or non-secure. If we remove secure >> channel option, what do we do on some other platform that needs it? >> And Linux may not always run in non-secure mode. > > > In general secure accesses are avoided these days in Linux as we have no > way to identify it. I agree there are few place where we do access. > Even though I don't like you have secure channel access in Linux, you > have valid reasons. In case you decide to support it, there is some > restrictions in bit 31, though RAZ/WI you need to notify that to protocol > if it tries to access it ? > > >> So here we populate all channels and let the clients knows which >> channel to use via platform specific details - DT. Please note the >> driver doesn't touch any secure resource if client doesn't ask it to >> (except SCFG for now, which I think should have some S vs NS DT >> binding). >> > > Not sure of this though. We need feedback from DT maintainers. > Yup. Cheers, Jassi
On 17/07/14 13:56, Jassi Brar wrote: > On 17 July 2014 16:01, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 17/07/14 07:25, Jassi Brar wrote: >>> >>>> >>>>> + u32 val; >>>>> + >>>>> + pr_debug("%s:%d\n", __func__, __LINE__); >>>> >>>> >>>> >>>> Please remove all these debug prints. >>>> >>> These are as good as absent unless DEBUG is defined. >>> >> >> Yes, but with mailbox framework, the controller driver(esp. this one)is >> almost like a shim layer. Instead of each driver adding these debugs, >> IMO it would be good to have these in the framework. >> > OK > >>>> >>>>> + /* See NOTE_RX_DONE */ >>>>> + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); >>>>> + mbox_chan_received_data(chan, (void *)val); >>>>> + >>>>> + /* >>>>> + * It is agreed with the remote firmware that the receiver >>>>> + * will clear the STAT register indicating it is ready to >>>>> + * receive next data - NOTE_RX_DONE >>>>> + */ >>>> >>>> >>>> This note could be added as how this mailbox works in general and >>>> it's not just Rx right ? Even Tx done is based on this logic. >>>> Basically the logic is valid on both directions. >>>> >>> Yes that is a protocol level agreement. Different f/w may behave >>> differently. >>> >> >> Ok, I am not sure if that's entirely true because the MHU spec says it >> drives >> the signal using a 32-bit register, with all 32 bits logically ORed >> together. >> Usually only Rx signals are wired to interrupts and Tx needs to be polled >> but that's entirely implementation choice I believe. >> > On my platform, _STAT register only carries the command code but > actual data is exchanged via SharedMemory/SHM. Now we need to know > when the sent data packet (in SHM) has been consumed by the remote - > for which our protocol mandates the remote clear the TX_STAT register. > And vice-versa for RX. > > Some other f/w may choose some other mechanism for TX-Done - say some > ACK bit set or even some time bound guarantee. > >> More over if it's protocol level agreement it should not belong here :) >> > My f/w expects the RX_STAT cleared after reading data from SHM. Where > do you think is the right place to clear RX_STAT? > I understand that and what I am saying is the MHU is designed like that and protocol is just using it. There's nothing specific to protocol. Ideally if an implementation has both Rx and Tx interrupts, the RX_CLEAR from here raises an interrupt to the firmware. In absence of it we need polling that's what both Linux and firmware does for Tx case. Even on Juno, it's same. But we should be able to extend it to support Tx if an implementation supports. Similarly the firmware can make use of the same when Linux clears Rx(it would be Tx complete/ack for the firmware) When we need to make this driver work across different firmware, you just can't rely on the firmware protocol, hence I am asking to drop those comments. > I have said many times in many threads... its the mailbox controller > _and_ the remote f/w that should be seen as one entity. It may not be > possible to write a controller driver that works with any remote > firmware. > It should be possible if the remote protocol just use the same hardware feature without any extra software policy at the lowest level(raw Tx and Rx). I believe that's what we need here if we want this driver to work on both Juno and your platform. Agree ? >> >>>>> +static int mhu_startup(struct mbox_chan *chan) >>>>> +{ >>>>> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >>>>> + unsigned long flags; >>>>> + u32 val; >>>>> + int ret; >>>>> + >>>>> + pr_debug("%s:%d\n", __func__, __LINE__); >>>>> + spin_lock_irqsave(&mlink->lock, flags); >>>>> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >>>>> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >>>>> + spin_unlock_irqrestore(&mlink->lock, flags); >>>>> + >>>>> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >>>>> + IRQF_SHARED, "mhu_link", chan); >>>> >>>> >>>> >>>> Just a thought: Can this be threaded_irq instead ? >>>> Can move request_irq to probe instead esp. if threaded_irq ? >>>> That provides some flexibility to client's rx_callback. >>>> >>> This is controller driver, and can not know which client want >>> rx_callback in hard-irq context and which in thread_fn context. >>> Otherwise, request_irq() does evaluate to request_threaded_irq(), if >>> thats what you mean. >> >> Agreed but on contrary since MHU involves external firmware(running >> on different processor) which might have it's own latency, I prefer >> threaded over hard irq. And yes request_irq does evaluate to >> request_threaded_irq but with thread_fn = NULL which is not what I want. >> > If remote has its own latency, that does not mean we add some more :) > No what I meant is unless there is a real need to use hard irq, we should prefer threaded one otherwise. Also by latency I meant what if the remote firmware misbehaves. In threaded version you have little more flexibility whereas hard irq is executed with interrupts disabled. At least the system is responsive and only MHU interrupts are disabled. Regards, Sudeep
On 17 July 2014 20:39, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 17/07/14 13:56, Jassi Brar wrote: >> >> On 17 July 2014 16:01, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>> On 17/07/14 07:25, Jassi Brar wrote: >>>> >>>> >>>>> >>>>>> + u32 val; >>>>>> + >>>>>> + pr_debug("%s:%d\n", __func__, __LINE__); >>>>> >>>>> >>>>> >>>>> >>>>> Please remove all these debug prints. >>>>> >>>> These are as good as absent unless DEBUG is defined. >>>> >>> >>> Yes, but with mailbox framework, the controller driver(esp. this one)is >>> almost like a shim layer. Instead of each driver adding these debugs, >>> IMO it would be good to have these in the framework. >>> >> OK >> >>>>> >>>>>> + /* See NOTE_RX_DONE */ >>>>>> + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); >>>>>> + mbox_chan_received_data(chan, (void *)val); >>>>>> + >>>>>> + /* >>>>>> + * It is agreed with the remote firmware that the receiver >>>>>> + * will clear the STAT register indicating it is ready to >>>>>> + * receive next data - NOTE_RX_DONE >>>>>> + */ >>>>> >>>>> >>>>> >>>>> This note could be added as how this mailbox works in general and >>>>> it's not just Rx right ? Even Tx done is based on this logic. >>>>> Basically the logic is valid on both directions. >>>>> >>>> Yes that is a protocol level agreement. Different f/w may behave >>>> differently. >>>> >>> >>> Ok, I am not sure if that's entirely true because the MHU spec says it >>> drives >>> the signal using a 32-bit register, with all 32 bits logically ORed >>> together. >>> Usually only Rx signals are wired to interrupts and Tx needs to be polled >>> but that's entirely implementation choice I believe. >>> >> On my platform, _STAT register only carries the command code but >> actual data is exchanged via SharedMemory/SHM. Now we need to know >> when the sent data packet (in SHM) has been consumed by the remote - >> for which our protocol mandates the remote clear the TX_STAT register. >> And vice-versa for RX. >> >> Some other f/w may choose some other mechanism for TX-Done - say some >> ACK bit set or even some time bound guarantee. >> >>> More over if it's protocol level agreement it should not belong here :) >>> >> My f/w expects the RX_STAT cleared after reading data from SHM. Where >> do you think is the right place to clear RX_STAT? >> > > I understand that and what I am saying is the MHU is designed like that > and protocol is just using it. There's nothing specific to protocol. Ideally > if an implementation has both Rx and Tx interrupts, the RX_CLEAR from here > raises an interrupt to the firmware. In absence of it we need polling that's > what both Linux and firmware does for Tx case. > > Even on Juno, it's same. But we should be able to extend it to support Tx > if an implementation supports. Similarly the firmware can make use of the > same when Linux clears Rx(it would be Tx complete/ack for the firmware) > > When we need to make this driver work across different firmware, you just > can't rely on the firmware protocol, hence I am asking to drop those > comments. > OK, I will remove the comment. > >> I have said many times in many threads... its the mailbox controller >> _and_ the remote f/w that should be seen as one entity. It may not be >> possible to write a controller driver that works with any remote >> firmware. >> > > It should be possible if the remote protocol just use the same hardware > feature without any extra software policy at the lowest level(raw Tx and > Rx). > I wouldn't count on f/w always done the right way. And I am speaking from my whatever first hand experience :D And sometimes there may just be bugs that need some quirks at controller level. > I believe that's what we need here if we want this driver to work > on both Juno and your platform. Agree ? > Does this driver not work for Juno? If no, may I see your driver and the MHU manual (mine is 90% Japanese)? > >>> >>>>>> +static int mhu_startup(struct mbox_chan *chan) >>>>>> +{ >>>>>> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >>>>>> + unsigned long flags; >>>>>> + u32 val; >>>>>> + int ret; >>>>>> + >>>>>> + pr_debug("%s:%d\n", __func__, __LINE__); >>>>>> + spin_lock_irqsave(&mlink->lock, flags); >>>>>> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >>>>>> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >>>>>> + spin_unlock_irqrestore(&mlink->lock, flags); >>>>>> + >>>>>> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >>>>>> + IRQF_SHARED, "mhu_link", chan); >>>>> >>>>> >>>>> >>>>> >>>>> Just a thought: Can this be threaded_irq instead ? >>>>> Can move request_irq to probe instead esp. if threaded_irq ? >>>>> That provides some flexibility to client's rx_callback. >>>>> >>>> This is controller driver, and can not know which client want >>>> rx_callback in hard-irq context and which in thread_fn context. >>>> Otherwise, request_irq() does evaluate to request_threaded_irq(), if >>>> thats what you mean. >>> >>> >>> Agreed but on contrary since MHU involves external firmware(running >>> on different processor) which might have it's own latency, I prefer >>> threaded over hard irq. And yes request_irq does evaluate to >>> request_threaded_irq but with thread_fn = NULL which is not what I want. >>> >> If remote has its own latency, that does not mean we add some more :) >> > > No what I meant is unless there is a real need to use hard irq, we > should prefer threaded one otherwise. > And how does controller discern a "real need" from a "soft need" to use hard irq? Even if the controller driver pushes data up from a threaded function, the client can't know the context and can't sleep because the intermediate API says the rx_callback should be assumed to be atomic. Again, it maybe more efficient if I see your implementation of the driver and understand your concerns about mine. > Also by latency I meant what if > the remote firmware misbehaves. In threaded version you have little > more flexibility whereas hard irq is executed with interrupts disabled. > At least the system is responsive and only MHU interrupts are disabled. > If the remote firmware misbehaves, that is a bug of the platform. No mailbox API could/should account for such malfunctions. Thanks Jassi
On 17/07/14 18:07, Jassi Brar wrote: > On 17 July 2014 20:39, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 17/07/14 13:56, Jassi Brar wrote: >>> >>> On 17 July 2014 16:01, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> [...] >>>>>> This note could be added as how this mailbox works in general and >>>>>> it's not just Rx right ? Even Tx done is based on this logic. >>>>>> Basically the logic is valid on both directions. >>>>>> >>>>> Yes that is a protocol level agreement. Different f/w may behave >>>>> differently. >>>>> >>>> >>>> Ok, I am not sure if that's entirely true because the MHU spec says it >>>> drives >>>> the signal using a 32-bit register, with all 32 bits logically ORed >>>> together. >>>> Usually only Rx signals are wired to interrupts and Tx needs to be polled >>>> but that's entirely implementation choice I believe. >>>> >>> On my platform, _STAT register only carries the command code but >>> actual data is exchanged via SharedMemory/SHM. Now we need to know >>> when the sent data packet (in SHM) has been consumed by the remote - >>> for which our protocol mandates the remote clear the TX_STAT register. >>> And vice-versa for RX. >>> >>> Some other f/w may choose some other mechanism for TX-Done - say some >>> ACK bit set or even some time bound guarantee. >>> >>>> More over if it's protocol level agreement it should not belong here :) >>>> >>> My f/w expects the RX_STAT cleared after reading data from SHM. Where >>> do you think is the right place to clear RX_STAT? >>> >> >> I understand that and what I am saying is the MHU is designed like that >> and protocol is just using it. There's nothing specific to protocol. Ideally >> if an implementation has both Rx and Tx interrupts, the RX_CLEAR from here >> raises an interrupt to the firmware. In absence of it we need polling that's >> what both Linux and firmware does for Tx case. >> >> Even on Juno, it's same. But we should be able to extend it to support Tx >> if an implementation supports. Similarly the firmware can make use of the >> same when Linux clears Rx(it would be Tx complete/ack for the firmware) >> >> When we need to make this driver work across different firmware, you just >> can't rely on the firmware protocol, hence I am asking to drop those >> comments. >> > OK, I will remove the comment. > >> >>> I have said many times in many threads... its the mailbox controller >>> _and_ the remote f/w that should be seen as one entity. It may not be >>> possible to write a controller driver that works with any remote >>> firmware. >>> >> >> It should be possible if the remote protocol just use the same hardware >> feature without any extra software policy at the lowest level(raw Tx and >> Rx). >> > I wouldn't count on f/w always done the right way. And I am speaking > from my whatever first hand experience :D > And sometimes there may just be bugs that need some quirks at controller level. > Agreed, and I too have similar experience. This is exact reason why I am urging for threaded irq, unless we have real requirement for hard irq. >> I believe that's what we need here if we want this driver to work >> on both Juno and your platform. Agree ? >> > Does this driver not work for Juno? I have not yet tried yet. For sure secure access will explode. > If no, may I see your driver and the MHU manual (mine is 90% Japanese)? > It's quite similar to your one expect few comments which I have made. Here is the public version of Juno spec[1]. Not sure if it covers MHU in detail. >> >>>> >>>>>>> +static int mhu_startup(struct mbox_chan *chan) >>>>>>> +{ >>>>>>> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >>>>>>> + unsigned long flags; >>>>>>> + u32 val; >>>>>>> + int ret; >>>>>>> + >>>>>>> + pr_debug("%s:%d\n", __func__, __LINE__); >>>>>>> + spin_lock_irqsave(&mlink->lock, flags); >>>>>>> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >>>>>>> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >>>>>>> + spin_unlock_irqrestore(&mlink->lock, flags); >>>>>>> + >>>>>>> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >>>>>>> + IRQF_SHARED, "mhu_link", chan); >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Just a thought: Can this be threaded_irq instead ? >>>>>> Can move request_irq to probe instead esp. if threaded_irq ? >>>>>> That provides some flexibility to client's rx_callback. >>>>>> >>>>> This is controller driver, and can not know which client want >>>>> rx_callback in hard-irq context and which in thread_fn context. >>>>> Otherwise, request_irq() does evaluate to request_threaded_irq(), if >>>>> thats what you mean. >>>> >>>> >>>> Agreed but on contrary since MHU involves external firmware(running >>>> on different processor) which might have it's own latency, I prefer >>>> threaded over hard irq. And yes request_irq does evaluate to >>>> request_threaded_irq but with thread_fn = NULL which is not what I want. >>>> >>> If remote has its own latency, that does not mean we add some more :) >>> >> >> No what I meant is unless there is a real need to use hard irq, we >> should prefer threaded one otherwise. >> > And how does controller discern a "real need" from a "soft need" to > use hard irq? > Even if the controller driver pushes data up from a threaded function, > the client can't know the context and can't sleep because the > intermediate API says the rx_callback should be assumed to be atomic. Yes I am not arguing on that, it should assume atomic and not sleep. I am saying we can avoid rx_callback in interrupt context if possible. I will try to look at the protocol implementation tomorrow. > Again, it maybe more efficient if I see your implementation of the > driver and understand your concerns about mine. > As I said its almost same as this, except I call mbox_chan_received_data in irq thread context. I prefer enabling other interrupts while copying payload data. >> Also by latency I meant what if >> the remote firmware misbehaves. In threaded version you have little >> more flexibility whereas hard irq is executed with interrupts disabled. >> At least the system is responsive and only MHU interrupts are disabled. >> > If the remote firmware misbehaves, that is a bug of the platform. No > mailbox API could/should account for such malfunctions. > No I didn't intend for any mailbox API to account it. Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.dto0038a/index.html
On 18 July 2014 00:21, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 17/07/14 18:07, Jassi Brar wrote: > >>> I believe that's what we need here if we want this driver to work >>> on both Juno and your platform. Agree ? >>> >> Does this driver not work for Juno? > > > I have not yet tried yet. For sure secure access will explode. > OK, I will remove setting up SCFG. >>> >>> No what I meant is unless there is a real need to use hard irq, we >>> should prefer threaded one otherwise. >>> >> And how does controller discern a "real need" from a "soft need" to >> use hard irq? >> Even if the controller driver pushes data up from a threaded function, >> the client can't know the context and can't sleep because the >> intermediate API says the rx_callback should be assumed to be atomic. > > Yes I am not arguing on that, it should assume atomic and not sleep. > I am saying we can avoid rx_callback in interrupt context if possible. > I will try to look at the protocol implementation tomorrow. > There is only one way for controller to push data to client... which is rx_callback() and it specified to be atomic. > >> Again, it maybe more efficient if I see your implementation of the >> driver and understand your concerns about mine. >> > > As I said its almost same as this, except I call mbox_chan_received_data > in irq thread context. I prefer enabling other interrupts while copying > payload data. > You call mbox_chan_received_data (which does rx_callback) from threaded handler. If your client only does atomic stuff in rx_callback(), then you pay for nothing. If your client does sleepable stuff then, as you agree, that's wrong. cheers -jassi
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index c8b5c13..681aac2 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -6,6 +6,13 @@ menuconfig MAILBOX signals. Say Y if your platform supports hardware mailboxes. if MAILBOX + +config MBOX_F_MHU + bool + depends on ARCH_MB86S7X + help + Say Y here if you want to use the F_MHU IPCM support. + config PL320_MBOX bool "ARM PL320 Mailbox" depends on ARM_AMBA diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 2fa343a..3707e93 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -2,6 +2,8 @@ obj-$(CONFIG_MAILBOX) += mailbox.o +obj-$(CONFIG_MBOX_F_MHU) += f_mhu.o + obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP_MBOX) += omap-mailbox.o diff --git a/drivers/mailbox/f_mhu.c b/drivers/mailbox/f_mhu.c new file mode 100644 index 0000000..cf5d3cd --- /dev/null +++ b/drivers/mailbox/f_mhu.c @@ -0,0 +1,227 @@ +/* + * Copyright (C) 2013-2014 Fujitsu Semiconductor Ltd. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/interrupt.h> +#include <linux/spinlock.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/mailbox_controller.h> +#include <linux/platform_device.h> + +#define INTR_STAT_OFS 0x0 +#define INTR_SET_OFS 0x8 +#define INTR_CLR_OFS 0x10 + +#define MHU_SCFG 0x400 + +struct mhu_link { + unsigned irq; + spinlock_t lock; /* channel regs */ + void __iomem *tx_reg; + void __iomem *rx_reg; +}; + +struct f_mhu { + void __iomem *base; + struct clk *clk; + struct mhu_link mlink[3]; + struct mbox_chan chan[3]; + struct mbox_controller mbox; +}; + +static irqreturn_t mhu_rx_interrupt(int irq, void *p) +{ + struct mbox_chan *chan = (struct mbox_chan *)p; + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; + u32 val; + + pr_debug("%s:%d\n", __func__, __LINE__); + /* See NOTE_RX_DONE */ + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); + mbox_chan_received_data(chan, (void *)val); + + /* + * It is agreed with the remote firmware that the receiver + * will clear the STAT register indicating it is ready to + * receive next data - NOTE_RX_DONE + */ + writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS); + + return IRQ_HANDLED; +} + +static bool mhu_last_tx_done(struct mbox_chan *chan) +{ + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; + unsigned long flags; + u32 val; + + pr_debug("%s:%d\n", __func__, __LINE__); + spin_lock_irqsave(&mlink->lock, flags); + /* See NOTE_RX_DONE */ + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); + spin_unlock_irqrestore(&mlink->lock, flags); + + return (val == 0); +} + +static int mhu_send_data(struct mbox_chan *chan, void *data) +{ + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; + unsigned long flags; + + pr_debug("%s:%d\n", __func__, __LINE__); + if (!mhu_last_tx_done(chan)) { + pr_err("%s:%d Shouldn't have seen the day!\n", + __func__, __LINE__); + return -EBUSY; + } + + spin_lock_irqsave(&mlink->lock, flags); + writel_relaxed((u32)data, mlink->tx_reg + INTR_SET_OFS); + spin_unlock_irqrestore(&mlink->lock, flags); + + return 0; +} + +static int mhu_startup(struct mbox_chan *chan) +{ + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; + unsigned long flags; + u32 val; + int ret; + + pr_debug("%s:%d\n", __func__, __LINE__); + spin_lock_irqsave(&mlink->lock, flags); + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); + spin_unlock_irqrestore(&mlink->lock, flags); + + ret = request_irq(mlink->irq, mhu_rx_interrupt, + IRQF_SHARED, "mhu_link", chan); + if (unlikely(ret)) { + pr_err("Unable to aquire IRQ\n"); + return ret; + } + + return 0; +} + +static void mhu_shutdown(struct mbox_chan *chan) +{ + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; + + pr_debug("%s:%d\n", __func__, __LINE__); + free_irq(mlink->irq, chan); +} + +static struct mbox_chan_ops mhu_ops = { + .send_data = mhu_send_data, + .startup = mhu_startup, + .shutdown = mhu_shutdown, + .last_tx_done = mhu_last_tx_done, +}; + +static int f_mhu_probe(struct platform_device *pdev) +{ + int i, err; + struct f_mhu *mhu; + struct resource *res; + int mhu_reg[3] = {0x0, 0x20, 0x200}; + + /* Allocate memory for device */ + mhu = kzalloc(sizeof(*mhu), GFP_KERNEL); + if (!mhu) { + dev_err(&pdev->dev, "failed to allocate memory.\n"); + return -EBUSY; + } + + mhu->clk = clk_get(&pdev->dev, "clk"); + if (unlikely(IS_ERR(mhu->clk))) { + dev_err(&pdev->dev, "unable to init clock\n"); + kfree(mhu); + return -EINVAL; + } + clk_prepare_enable(mhu->clk); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + mhu->base = ioremap(res->start, resource_size(res)); + if (!mhu->base) { + dev_err(&pdev->dev, "ioremap failed.\n"); + kfree(mhu); + return -EBUSY; + } + + /* Let UnTrustedOS's access violations don't bother us */ + writel_relaxed(0, mhu->base + MHU_SCFG); + + for (i = 0; i < 3; i++) { + mhu->chan[i].con_priv = &mhu->mlink[i]; + spin_lock_init(&mhu->mlink[i].lock); + res = platform_get_resource(pdev, IORESOURCE_IRQ, i); + mhu->mlink[i].irq = res->start; + mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i]; + mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + 0x100; + } + + mhu->mbox.dev = &pdev->dev; + mhu->mbox.chans = &mhu->chan[0]; + mhu->mbox.num_chans = 3; + mhu->mbox.ops = &mhu_ops; + mhu->mbox.txdone_irq = false; + mhu->mbox.txdone_poll = true; + mhu->mbox.txpoll_period = 10; + + platform_set_drvdata(pdev, mhu); + + err = mbox_controller_register(&mhu->mbox); + if (err) { + dev_err(&pdev->dev, "Failed to register mailboxes %d\n", err); + iounmap(mhu->base); + kfree(mhu); + } else { + dev_info(&pdev->dev, "Fujitsu MHU Mailbox registered\n"); + } + + return 0; +} + +static const struct of_device_id f_mhu_dt_ids[] = { + { .compatible = "fujitsu,mhu" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, f_mhu_dt_ids); + +static struct platform_driver f_mhu_driver = { + .driver = { + .name = "f_mhu", + .owner = THIS_MODULE, + .of_match_table = f_mhu_dt_ids, + }, + .probe = f_mhu_probe, +}; + +static int __init f_mhu_init(void) +{ + return platform_driver_register(&f_mhu_driver); +} +module_init(f_mhu_init); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Fujitsu MHU Driver"); +MODULE_AUTHOR("Jassi Brar <jaswinder.singh@linaro.org>");