mailbox: f_mhu: add driver for Fujitsu MHU controller
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Add driver for the proprietary Mailbox controller (f_mhu) in MB86S7x. It has three channels - LowPri-NonSecure, HighPri-NonSecure and Secure. The MB86S7x communicates over the HighPri-NonSecure channel.
--- 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
drivers/mailbox/Kconfig +7 -0
@@ -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
drivers/mailbox/Makefile +2 -0
@@ -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
drivers/mailbox/f_mhu.c new file +227 -0
227 lines not on this page · raw message
Signature · 1 line
1.8.1.2
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Hi Mollie,
Quoted 1 line · Mollie Wu
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 :)
Quoted 18 lines
It has three channels - LowPri-NonSecure, HighPri-NonSecure and Secure. The MB86S7x communicates over the HighPri-NonSecure channel.
Signed-off-by Jassi Brar <[email protected]>Signed-off-by Tetsuya Takinishi <[email protected]>Signed-off-by Mollie Wu <[email protected]>--- 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.cdrivers/mailbox/Kconfig +0
-0
drivers/mailbox/Kconfig menuconfig MAILBOX · line 11
Quoted 6 lines
drivers/mailbox/Kconfig +3
-0@@ -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.
drivers/mailbox/Kconfig menuconfig MAILBOX · line 12
Quoted 1 line
drivers/mailbox/Kconfig +1
-0+ depends on ARCH_MB86S7X
Definitely not a requirement
drivers/mailbox/Kconfig menuconfig MAILBOX · line 15
Quoted 3 lines
drivers/mailbox/Kconfig +3
-0+ help + Say Y here if you want to use the F_MHU IPCM support. +
Also it needs some description.
Quoted 43 lines
drivers/mailbox/Kconfig +0
-0config PL320_MBOX bool "ARM PL320 Mailbox" depends on ARM_AMBAdrivers/mailbox/Makefile +2
-0@@ -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.odrivers/mailbox/f_mhu.c new file +21
-0@@ -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>
drivers/mailbox/f_mhu.c line 31
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-0+#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)
Quoted 9 lines
drivers/mailbox/f_mhu.c new file +9
-0+struct mhu_link { +unsigned irq; +spinlock_t lock;/* channel regs */ +void __iomem *tx_reg; +void __iomem *rx_reg; +}; + +struct f_mhu { +void __iomem *base;
drivers/mailbox/f_mhu.c line 50
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-0+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
drivers/mailbox/f_mhu.c line 53
Quoted 3 lines
drivers/mailbox/f_mhu.c new file +3
-0+u32 val; + +pr_debug ("%s:%d\n" , __func__, __LINE__);
Please remove all these debug prints.
drivers/mailbox/f_mhu.c line 62
Quoted 9 lines
drivers/mailbox/f_mhu.c new file +9
-0+/* 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.
drivers/mailbox/f_mhu.c line 75
Quoted 13 lines
drivers/mailbox/f_mhu.c new file +13
-0+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.
Quoted 31 lines
drivers/mailbox/f_mhu.c new file +31
-0+/* 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;
drivers/mailbox/f_mhu.c line 116
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-0+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.
Quoted 19 lines
drivers/mailbox/f_mhu.c new file +19
-0+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,
drivers/mailbox/f_mhu.c line 145
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-0+ .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 ?
drivers/mailbox/f_mhu.c line 156
Quoted 11 lines
drivers/mailbox/f_mhu.c new file +11
-0+ +/* 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.
Quoted 6 lines
drivers/mailbox/f_mhu.c new file +6
-0+kfree (mhu); +return -EINVAL ; + } +clk_prepare_enable (mhu->clk); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );
drivers/mailbox/f_mhu.c line 172
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-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 ?
drivers/mailbox/f_mhu.c line 184
Quoted 12 lines
drivers/mailbox/f_mhu.c new file +12
-0+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.
Quoted 9 lines
drivers/mailbox/f_mhu.c new file +9
-0+ 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) {
drivers/mailbox/f_mhu.c line 203
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-0+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.
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-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 ,
drivers/mailbox/f_mhu.c line 223
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +10
-0+ .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
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Quoted 5 lines · Sudeep Holla
Hi Mollie,
Quoted 1 line · Mollie Wu
Quoted 8 lines
Quoted 1 line
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 :)
Quoted 8 lines
Can you please confirm this by reading Peripheral
ID(PID: 0xFD0, 0xFE0 - 0xFEC)and Component ID(COMPID:0xFF0-0xFFC). Are theyPID -
0x040x980xB00x1B0x00
COMPID -0x0D0xF00x050xB1If so we have do s/f_mhu/arm_mhu/g :)
Yup, we have to.
Quoted 22 lines
Quoted 22 lines
It has three channels - LowPri-NonSecure, HighPri-NonSecure and Secure. The MB86S7x communicates over the HighPri-NonSecure channel.
Signed-off-by Jassi Brar <[email protected]>Signed-off-by Tetsuya Takinishi <[email protected]>Signed-off-by Mollie Wu <[email protected]>--- 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.cdrivers/mailbox/Kconfig +0
-0@@ -6,6 +6,13 @@ menuconfig MAILBOX signals. Say Y if your platform supports hardware mailboxes.
Quoted 8 lines
drivers/mailbox/Kconfig menuconfig MAILBOX · line 11
Quoted 4 lines
drivers/mailbox/Kconfig +3
-0if MAILBOX + +config MBOX_F_MHU + boolOn 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.
Quoted 5 lines
drivers/mailbox/Kconfig menuconfig MAILBOX · line 12
Quoted 1 line
drivers/mailbox/Kconfig +1
-0+ depends on ARCH_MB86S7XDefinitely not a requirement
It was, until we found each other ;)
Quoted 8 lines
drivers/mailbox/Kconfig menuconfig MAILBOX · line 15
Quoted 4 lines
drivers/mailbox/Kconfig +3
-0+ help + Say Y here if you want to use the F_MHU IPCM support. +Also it needs some description.
OK
Quoted 10 lines
drivers/mailbox/Kconfig menuconfig MAILBOX
Quoted 6 lines
drivers/mailbox/Kconfig +6
-0+#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.
Quoted 15 lines
Quoted 15 lines
drivers/mailbox/Kconfig +14
-0+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; +};
Quoted 9 lines
drivers/mailbox/Kconfig
Quoted 5 lines
drivers/mailbox/Kconfig +5
-0+ +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
Quoted 8 lines
drivers/mailbox/Kconfig
Quoted 4 lines
drivers/mailbox/Kconfig +3
-0+ 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.
Quoted 11 lines
drivers/mailbox/Kconfig
Quoted 10 lines
drivers/mailbox/Kconfig +9
-0+ /* 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 + */
Quoted 4 lines
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.
Quoted 20 lines
Quoted 4 lines
drivers/mailbox/Kconfig +3
-0+ writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS); + + return IRQ_HANDLED;drivers/mailbox/Kconfig
Quoted 10 lines
drivers/mailbox/Kconfig +10
-0+} + +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_dataIIRC. And this function is just reading status do we really need the lock ?
Quoted 3 lines
I must be missing something here else IMO we can get rid of this extra locks in here.
Yeah, probably unnecessary.
Quoted 38 lines
Quoted 38 lines
drivers/mailbox/Kconfig +38
-0+ /* 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);
Quoted 9 lines
drivers/mailbox/Kconfig
Quoted 3 lines
drivers/mailbox/Kconfig +3
-0+ + ret = request_irq(mlink->irq, mhu_rx_interrupt, + IRQF_SHARED, "mhu_link", chan);Just a thought: Can this be
threaded_irqinstead ? Can moverequest_irqto probe instead esp. ifthreaded_irq? That provides some flexibility to client'srx_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.
Quoted 25 lines
Quoted 25 lines
drivers/mailbox/Kconfig +24
-0+ 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)
Quoted 9 lines
drivers/mailbox/Kconfig
Quoted 5 lines
drivers/mailbox/Kconfig +5
-0+{ + 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 :)
Quoted 17 lines
drivers/mailbox/Kconfig
Quoted 12 lines
drivers/mailbox/Kconfig +11
-0+ + /* 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.
Quoted 13 lines
Quoted 13 lines
drivers/mailbox/Kconfig +12
-0+ 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; + }
Quoted 9 lines
drivers/mailbox/Kconfig
Quoted 4 lines
drivers/mailbox/Kconfig +4
-0+ + /* 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.
Quoted 2 lines
I might be missing to see, is there a binding document for this mhu ?
Yeah we need one.
Quoted 15 lines
drivers/mailbox/Kconfig
Quoted 13 lines
drivers/mailbox/Kconfig +12
-0+ 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;
Quoted 3 lines
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).
Quoted 16 lines
Quoted 16 lines
drivers/mailbox/Kconfig +10
-0+ 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);
drivers/mailbox/Kconfig +4
-0+ iounmap(mhu->base); + kfree(mhu); + } else { + dev_info(&pdev->dev, "Fujitsu MHU Mailbox registered\n");
Quoted 9 lines
drivers/mailbox/Kconfig
Quoted 5 lines
drivers/mailbox/Kconfig +5
-0+ } + + return 0; +} +Also to be module you need add remove.
Yup, OK.
Quoted 16 lines
Quoted 16 lines
drivers/mailbox/Kconfig +15
-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, +}; +
Quoted 9 lines
drivers/mailbox/Kconfig
Quoted 5 lines
drivers/mailbox/Kconfig +5
-0+static int __init f_mhu_init(void) +{ + return platform_driver_register(&f_mhu_driver); +} +module_init(f_mhu_init);This can be
module_platform_driverinstead.
OK
Thanks
-Jassi
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Quoted 7 lines · Jassi Brar
Quoted 6 lines · Sudeep Holla
Hi Mollie,
Quoted 2 lines · Mollie Wu
Add driver for the proprietary Mailbox controller (
f_mhu) in MB86S7x.
[...]
Quoted 10 lines
Quoted 8 lines
drivers/mailbox/f_mhu.c line 53
Quoted 4 lines
drivers/mailbox/f_mhu.c new file +3
-0+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.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +9
-0+/* 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 +*/
Quoted 6 lines
Quoted 4 lines
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 :)
Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
drivers/mailbox/f_mhu.c new file +12
-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);Quoted 9 lines
drivers/mailbox/f_mhu.c line 116
Quoted 3 lines
drivers/mailbox/f_mhu.c new file +3
-0+ + ret =request_irq (mlink->irq, mhu_rx_interrupt, +IRQF_SHARED ,"mhu_link" , chan);Just a thought: Can this be
threaded_irqinstead ? Can moverequest_irqto probe instead esp. ifthreaded_irq? That provides some flexibility to client'srx_callback.
Quoted 4 lines
This is controller driver, and can not know which client want
rx_callbackin hard-irq context and which inthread_fncontext. Otherwise,request_irq()does evaluate torequest_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.
Quoted 4 lines
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 arx_callbackin hard-irq context.
OK
Quoted 15 lines
Quoted 15 lines
Quoted 13 lines
drivers/mailbox/f_mhu.c new file +12
-0+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 ;
Quoted 7 lines
Quoted 3 lines
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 ?
Quoted 6 lines
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
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Quoted 9 lines · Sudeep Holla
Quoted 8 lines · Jassi Brar
Quoted 8 lines
Quoted 5 lines
drivers/mailbox/f_mhu.c new file +3
-0+u32 val; + +pr_debug ("%s:%d\n" , __func__, __LINE__);
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
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
Quoted 20 lines
Quoted 12 lines
Quoted 12 lines
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +9
-0+/* 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 +*/ Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 2 lines
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.
Quoted 28 lines
Quoted 23 lines
Quoted 14 lines
Quoted 14 lines
drivers/mailbox/f_mhu.c new file +13
-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); +Quoted 9 lines
drivers/mailbox/f_mhu.c line 116
Quoted 2 lines
drivers/mailbox/f_mhu.c new file +2
-0+ ret =request_irq (mlink->irq, mhu_rx_interrupt, +IRQF_SHARED ,"mhu_link" , chan);Just a thought: Can this be
threaded_irqinstead ? Can moverequest_irqto probe instead esp. ifthreaded_irq? That provides some flexibility to client'srx_callback.Quoted 4 lines
This is controller driver, and can not know which client want
rx_callbackin hard-irq context and which inthread_fncontext. Otherwise,request_irq()does evaluate torequest_threaded_irq(), if thats what you mean.
Quoted 5 lines
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_irqdoes evaluate torequest_threaded_irqbut withthread_fn= NULL which is not what I want.
If remote has its own latency, that does not mean we add some more :)
Quoted 6 lines
Quoted 4 lines
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 arx_callbackin hard-irq context.OK
Cool.
Quoted 34 lines
Quoted 17 lines
Quoted 17 lines
Quoted 14 lines
drivers/mailbox/f_mhu.c new file +12
-0+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 ;Quoted 7 lines
Quoted 3 lines
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 ?
Quoted 9 lines
Quoted 6 lines
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
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Quoted 54 lines · Jassi Brar
Quoted 9 lines · Sudeep Holla
Quoted 8 lines · Jassi Brar
Quoted 8 lines
Quoted 5 lines
drivers/mailbox/f_mhu.c new file +3
-0+u32 val; + +pr_debug ("%s:%d\n" , __func__, __LINE__);Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
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
Quoted 20 lines
Quoted 12 lines
Quoted 12 lines
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +9
-0+/* 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 +*/ Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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,
_STATregister 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 theTX_STATregister. And vice-versa for RX.
Quoted 8 lines
Some other f/w may choose some other mechanism for TX-Done - say some
ACK bit set or even some time bound guarantee.Quoted 2 lines
More over if it's protocol level agreement it should not belong here :)
My f/w expects the
RX_STATcleared after reading data from SHM. Where do you think is the right place to clearRX_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.
Quoted 5 lines
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 ?
Quoted 28 lines
Quoted 28 lines
Quoted 27 lines
Quoted 14 lines
Quoted 14 lines
drivers/mailbox/f_mhu.c new file +13
-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); +Quoted 9 lines
drivers/mailbox/f_mhu.c line 116
Quoted 2 lines
drivers/mailbox/f_mhu.c new file +2
-0+ ret =request_irq (mlink->irq, mhu_rx_interrupt, +IRQF_SHARED ,"mhu_link" , chan);Just a thought: Can this be
threaded_irqinstead ? Can moverequest_irqto probe instead esp. ifthreaded_irq? That provides some flexibility to client'srx_callback.This is controller driver, and can not know which client want
rx_callbackin hard-irq context and which inthread_fncontext. Otherwise,request_irq()does evaluate torequest_threaded_irq(), if thats what you mean.
Quoted 7 lines
Quoted 5 lines
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_irqdoes evaluate torequest_threaded_irqbut withthread_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
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Quoted 77 lines · Sudeep Holla
Quoted 59 lines · Jassi Brar
Quoted 12 lines · Sudeep Holla
Quoted 10 lines · Jassi Brar
Quoted 10 lines
Quoted 6 lines
drivers/mailbox/f_mhu.c new file +3
-0+u32 val; + +pr_debug ("%s:%d\n" , __func__, __LINE__);Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
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
Quoted 21 lines
Quoted 13 lines
Quoted 13 lines
Quoted 10 lines
drivers/mailbox/f_mhu.c new file +9
-0+/* 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 +*/ Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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,
_STATregister 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 theTX_STATregister. And vice-versa for RX.Quoted 8 lines
Some other f/w may choose some other mechanism for TX-Done - say some
ACK bit set or even some time bound guarantee.Quoted 2 lines
More over if it's protocol level agreement it should not belong here :)
My f/w expects the
RX_STATcleared after reading data from SHM. Where do you think is the right place to clearRX_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_CLEARfrom 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.
Quoted 8 lines
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.
Quoted 11 lines
Quoted 6 lines
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.
Quoted 3 lines
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)?
Quoted 36 lines
Quoted 36 lines
Quoted 36 lines
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
drivers/mailbox/f_mhu.c new file +14
-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,Quoted 9 lines
Quoted 1 line
+
IRQF_SHARED, "mhu_link", chan);Just a thought: Can this be
threaded_irqinstead ? Can moverequest_irqto probe instead esp. ifthreaded_irq? That provides some flexibility to client'srx_callback.Quoted 4 lines
This is controller driver, and can not know which client want
rx_callbackin hard-irq context and which inthread_fncontext. Otherwise,request_irq()does evaluate torequest_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_irqdoes evaluate torequest_threaded_irqbut withthread_fn= NULL which is not what I want.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
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
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Quoted 7 lines · Jassi Brar
Quoted 6 lines · Sudeep Holla
Quoted 3 lines · Jassi Brar
Quoted 1 line · Sudeep Holla
[...]
Quoted 53 lines
Quoted 36 lines
Quoted 21 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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,
_STATregister 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 theTX_STATregister. And vice-versa for RX.Quoted 8 lines
Some other f/w may choose some other mechanism for TX-Done - say some
ACK bit set or even some time bound guarantee.Quoted 2 lines
More over if it's protocol level agreement it should not belong here :)
My f/w expects the
RX_STATcleared after reading data from SHM. Where do you think is the right place to clearRX_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_CLEARfrom 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.Quoted 8 lines
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.
Quoted 7 lines
Quoted 6 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 4 lines
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 39 lines
Quoted 39 lines
Quoted 38 lines
Quoted 31 lines
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
(quoted diff) +14
-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,Quoted 9 lines
Quoted 1 line
+
IRQF_SHARED, "mhu_link", chan);Just a thought: Can this be
threaded_irqinstead ? Can moverequest_irqto probe instead esp. ifthreaded_irq? That provides some flexibility to client'srx_callback.Quoted 4 lines
This is controller driver, and can not know which client want
rx_callbackin hard-irq context and which inthread_fncontext. Otherwise,request_irq()does evaluate torequest_threaded_irq(), if thats what you mean.Quoted 5 lines
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_irqdoes evaluate torequest_threaded_irqbut withthread_fn= NULL which is not what I want.If remote has its own latency, that does not mean we add some more :)
Quoted 8 lines
Quoted 3 lines
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 therx_callbackshould 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.
Quoted 3 lines
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.
Quoted 8 lines
Quoted 5 lines
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
PATCH 7/8 mailbox: f_mhu: add driver for Fujitsu MHU controller
Quoted 11 lines · Sudeep Holla
Quoted 4 lines · Jassi Brar
Quoted 3 lines
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.
Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
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 therx_callbackshould be assumed to be atomic.
Quoted 4 lines
Yes I am not arguing on that, it should assume atomic and not sleep. I am saying we can avoid
rx_callbackin 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.
Quoted 9 lines
Quoted 4 lines
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_datain 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