| Message ID | 1425466884-30648-1-git-send-email-vincent.yang@socionext.com |
|---|---|
| State | Accepted, archived |
| Commit | ee23d66af9219dfe2407a9b08ef9a165dbe6f994 |
| Headers | show |
Hi Vincent, I see that this driver is queued but most of my earlier comments were not addressed. *No ACK from DT binding maintainers too*. On 04/03/15 11:01, Vincent Yang wrote: > From: Jassi Brar <jaswinder.singh@linaro.org> > > Add driver for the ARM Primecell Message-Handling-Unit(MHU) controller. > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > Signed-off-by: Andy Green <andy.green@linaro.org> > Signed-off-by: Vincent Yang <vincent.yang@socionext.com> > Signed-off-by: Tetsuya Nuriya <nuriya.tetsuya@socionext.com> > --- > .../devicetree/bindings/mailbox/arm-mhu.txt | 43 +++++ > drivers/mailbox/Kconfig | 9 + > drivers/mailbox/Makefile | 2 + > drivers/mailbox/arm_mhu.c | 195 +++++++++++++++++++++ > 4 files changed, 249 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt > create mode 100644 drivers/mailbox/arm_mhu.c > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > new file mode 100644 > index 0000000..4971f03 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > @@ -0,0 +1,43 @@ > +ARM MHU Mailbox Driver > +====================== > + > +The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has > +3 independent channels/links to communicate with remote processor(s). > + MHU links are hardwired on a platform. A link raises interrupt for any As I had mentioned before it's just one implementation, the IP had provision bi-directional interrupts and the binding *has to consider* that instead of having to workaround that later. > +received data. However, there is no specified way of knowing if the sent You can drop the above line or specify as you can still poll. > +data has been read by the remote. This driver assumes the sender polls Bindings not supposed have any details on how driver handle it. You can just mention that in absence of interrupt, we can know the status of transmission by polling STAT register. > +STAT register and the remote clears it after having read the data. > +The last channel is specified to be a 'Secure' resource, hence can't be > +used by Linux running NS. > + Correct so drop the support for secure channel until the first user comes up. I am worried it might generate abort if someone tries to access it in ARM64 which always runs in non-secure. > +Mailbox Device Node: > +==================== > + > +Required properties: > +-------------------- > +- compatible: Shall be "arm,mhu" & "arm,primecell" > +- reg: Contains the mailbox register address range (base > + address and length) > +- #mbox-cells Shall be 1 - the index of the channel needed. > +- interrupts: Contains the interrupt information corresponding to > + each of the 3 links of MHU. > + As mentioned multiple times before, please add interrupt-names to make sure we can handle bi-directional interrupt. > +Example: > +-------- > + > + mhu: mailbox@2b1f0000 { > + #mbox-cells = <1>; > + compatible = "arm,mhu", "arm,primecell"; > + reg = <0 0x2b1f0000 0x1000>; > + interrupts = <0 36 4>, /* LP-NonSecure */ > + <0 35 4>, /* HP-NonSecure */ > + <0 37 4>; /* Secure */ > + clocks = <&clock 0 2 1>; > + clock-names = "apb_pclk"; > + }; > + > + mhu_client: scb@2e000000 { > + compatible = "fujitsu,mb86s70-scb-1.0"; > + reg = <0 0x2e000000 0x4000>; > + mboxes = <&mhu 1>; /* HP-NonSecure */ > + }; [...] > diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c > new file mode 100644 > index 0000000..ac693c6 > --- /dev/null > +++ b/drivers/mailbox/arm_mhu.c > @@ -0,0 +1,195 @@ > +/* > + * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd. > + * Copyright (C) 2015 Linaro Ltd. > + * Author: Jassi Brar <jaswinder.singh@linaro.org> > + * > + * 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/module.h> > +#include <linux/amba/bus.h> > +#include <linux/mailbox_controller.h> > + > +#define INTR_STAT_OFS 0x0 > +#define INTR_SET_OFS 0x8 > +#define INTR_CLR_OFS 0x10 > + > +#define MHU_LP_OFFSET 0x0 > +#define MHU_HP_OFFSET 0x20 > +#define MHU_SEC_OFFSET 0x200 > +#define TX_REG_OFFSET 0x100 > + > +#define MHU_CHANS 3 > + For now I prefer 2. [...] > +static int mhu_probe(struct amba_device *adev, const struct amba_id *id) > +{ > + int i, err; > + struct arm_mhu *mhu; > + struct device *dev = &adev->dev; > + int mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET}; > + > + /* Allocate memory for device */ > + mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL); > + if (!mhu) > + return -ENOMEM; > + > + mhu->base = devm_ioremap_resource(dev, &adev->res); > + if (IS_ERR(mhu->base)) { > + dev_err(dev, "ioremap failed\n"); > + return PTR_ERR(mhu->base); > + } > + > + for (i = 0; i < MHU_CHANS; i++) { > + mhu->chan[i].con_priv = &mhu->mlink[i]; > + mhu->mlink[i].irq = adev->irq[i]; > + mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i]; > + mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET; > + } > + > + mhu->mbox.dev = dev; > + mhu->mbox.chans = &mhu->chan[0]; > + mhu->mbox.num_chans = MHU_CHANS; > + mhu->mbox.ops = &mhu_ops; > + mhu->mbox.txdone_irq = false; > + mhu->mbox.txdone_poll = true; > + mhu->mbox.txpoll_period = 10; IMO too high value, as I mentioned you assume jiffy value. The kernel will take care of conversion. [...] > + > +static struct amba_id mhu_ids[] = { > + { > + .id = 0x1bb098, As I mentioned couple of times this is broken. Please refer my earlier mail for details. You are skipping a field to compare which incorrect. You are just trying to get it working with existing AMBA driver without any changes matching the IDs partially. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vincent, Forgot couple of things earlier: On 04/03/15 11:01, Vincent Yang wrote: > From: Jassi Brar <jaswinder.singh@linaro.org> > > Add driver for the ARM Primecell Message-Handling-Unit(MHU) controller. > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > Signed-off-by: Andy Green <andy.green@linaro.org> > Signed-off-by: Vincent Yang <vincent.yang@socionext.com> > Signed-off-by: Tetsuya Nuriya <nuriya.tetsuya@socionext.com> > --- > .../devicetree/bindings/mailbox/arm-mhu.txt | 43 +++++ > drivers/mailbox/Kconfig | 9 + > drivers/mailbox/Makefile | 2 + > drivers/mailbox/arm_mhu.c | 195 +++++++++++++++++++++ > 4 files changed, 249 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt > create mode 100644 drivers/mailbox/arm_mhu.c > [...] > +static int mhu_send_data(struct mbox_chan *chan, void *data) > +{ > + struct mhu_link *mlink = chan->con_priv; > + u32 *arg = data; > + Arnd doesn't like this and had suggestions in some other thread. > + writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS); > + > + return 0; > +} > + > +static int mhu_startup(struct mbox_chan *chan) > +{ > + struct mhu_link *mlink = chan->con_priv; > + u32 val; > + int ret; > + > + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); > + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); > + > + ret = request_irq(mlink->irq, mhu_rx_interrupt, > + IRQF_SHARED, "mhu_link", chan); Any reason we can't move this to probe and have {en,dis}able_irq here if needed. I has seen it was too heavy to have these especially when sending small packets. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 18, 2015 at 3:27 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > Hi Vincent, > > I see that this driver is queued but most of my earlier comments were > not addressed. > Some of your comments have been addressed as changes and some as replies. > *No ACK from DT binding maintainers too*. > Yeah it would have been great to have some ack. It has been many months now, but I see its not uncommon for bindings go in via maintainers. I assume folks find it just normal. >> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt >> @@ -0,0 +1,43 @@ >> +ARM MHU Mailbox Driver >> +====================== >> + >> +The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has >> +3 independent channels/links to communicate with remote processor(s). >> + MHU links are hardwired on a platform. A link raises interrupt for any > > > As I had mentioned before it's just one implementation, the IP had > provision bi-directional interrupts and the binding *has to consider* > that instead of having to workaround that later. > I will reply in yet another way.... the spec I have don't mention that (and it makes sense) and your platform doesn't do that either. I would have had to do something about it, had there been such a weird setup, but there's none known. So let us please avoid getting into bikeshedding discussion yet again. Otherwise almost every binding is broken because there could always be some platform that does weird things with irqs. >> +The last channel is specified to be a 'Secure' resource, hence can't be >> +used by Linux running NS. > > Correct so drop the support for secure channel until the first user > comes up. > In your last post you said: "Sorry my statement was not clear, I wanted to ask you to add that info to the binding." Not that it makes sense to say the obvious, I added it just to address your comment. > I am worried it might generate abort if someone tries to > access it in ARM64 which always runs in non-secure. > Abort should have happened years ago if someone _tries_ to access a secure resource from non-secure mode ;) How do you protect such people from "trying to" dereference NULL pointers? (you made me repeat Nth time). >> + >> +static struct amba_id mhu_ids[] = { >> + { >> + .id = 0x1bb098, > > > As I mentioned couple of times this is broken. Please refer my earlier mail > for details. You are skipping a field to compare which incorrect. > You are just trying to get it working with existing AMBA driver without any > changes matching the IDs partially. > I did not miss your last mail. Let me try to reply in even simpler terms... PID[0,3] identification is fine grained enough. If/when we have some platform with non-ARM MHU, we could catch that in this driver. Most likely even then the reg-map won't change drastically, if at all. So we could ignore the PID4, otherwise we check PID4 in this driver and adapt the behavior. -Jassi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 18, 2015 at 3:55 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> +static int mhu_send_data(struct mbox_chan *chan, void *data) >> +{ >> + struct mhu_link *mlink = chan->con_priv; >> + u32 *arg = data; > > Arnd doesn't like this and had suggestions in some other thread. > No, Arnd suggested doing it this way. And another platform's driver was made to do this way. >> + writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS); >> + >> + return 0; >> +} >> + >> +static int mhu_startup(struct mbox_chan *chan) >> +{ >> + struct mhu_link *mlink = chan->con_priv; >> + u32 val; >> + int ret; >> + >> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >> + >> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >> + IRQF_SHARED, "mhu_link", chan); > > > Any reason we can't move this to probe and have {en,dis}able_irq here if > needed. I has seen it was too heavy to have these especially when > sending small packets. > I see you used to do memcpy in irq-handler https://git.linaro.org/landing-teams/working/arm/kernel.git/blob/HEAD:/drivers/mailbox/arm_mhu.c perhaps you were using your old driver? If you use this new driver, and send packets so often that request-release irq has effect, maybe should hold the mailbox reference for lifetime. I remember suggesting you that already and I remember you said that's how it was. -Jassi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/03/15 12:56, Jassi Brar wrote: > On Wed, Mar 18, 2015 at 3:27 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Hi Vincent, >> >> I see that this driver is queued but most of my earlier comments were >> not addressed. >> > Some of your comments have been addressed as changes and some as replies. > >> *No ACK from DT binding maintainers too*. >> > Yeah it would have been great to have some ack. It has been many > months now, but I see its not uncommon for bindings go in via > maintainers. I assume folks find it just normal. > OK, but I have one concern in the binding as mentioned previous and again as below. >>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt >>> @@ -0,0 +1,43 @@ >>> +ARM MHU Mailbox Driver >>> +====================== >>> + >>> +The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has >>> +3 independent channels/links to communicate with remote processor(s). >>> + MHU links are hardwired on a platform. A link raises interrupt for any >> >> >> As I had mentioned before it's just one implementation, the IP had >> provision bi-directional interrupts and the binding *has to consider* >> that instead of having to workaround that later. >> > I will reply in yet another way.... the spec I have don't mention that > (and it makes sense) and your platform doesn't do that either. I would > have had to do something about it, had there been such a weird setup, > but there's none known. > Not just weird setup, even extending Tx support with this binding is not straight forward. Simple example: Today you have: interrupts = <0 36 4>, /* LP-NonSecure */ <0 35 4>, /* HP-NonSecure */ <0 37 4>; /* Secure */ I don't want to specify Secure IRQ on my platform, which should be fine. In future a variant of that platform gets Tx interrupts, then I will add something like below: interrupts = <0 36 4>, /* LP-NonSecure Rx */ <0 35 4>, /* HP-NonSecure Rx */ <0 39 4>, /* LP-NonSecure Tx */ <0 38 4>, /* HP-NonSecure Tx */ Now how will we handle this ? I am just suggesting to use interrupt-names so that we don't depend on ordering in DT implicitly from first by fixing the binding. > So let us please avoid getting into bikeshedding discussion yet > again. Otherwise almost every binding is broken because there could > always be some platform that does weird things with irqs. > Sorry I don't buy that given that using interrupt-names simplifies things and also helps to hand any such cases. >>> +The last channel is specified to be a 'Secure' resource, hence can't be >>> +used by Linux running NS. >> >> Correct so drop the support for secure channel until the first user >> comes up. >> > In your last post you said: > "Sorry my statement was not clear, I wanted to ask you to add that info > to the binding." > Not that it makes sense to say the obvious, I added it just to > address your comment. > >> I am worried it might generate abort if someone tries to >> access it in ARM64 which always runs in non-secure. >> > Abort should have happened years ago if someone _tries_ to access a > secure resource from non-secure mode ;) > How do you protect such people from "trying to" dereference NULL > pointers? (you made me repeat Nth time). > When the general practice everywhere in the kernel is to avoid secure accesses *unless and until absolutely necessary*, I don't understand *why exactly we need the exception here* ? I am OK if you add that justification with the first user of that channel. We have seen several cases/issues around that and I am just looking for a strong reason to add secure channel support here. >>> + >>> +static struct amba_id mhu_ids[] = { >>> + { >>> + .id = 0x1bb098, >> >> >> As I mentioned couple of times this is broken. Please refer my earlier mail >> for details. You are skipping a field to compare which incorrect. >> You are just trying to get it working with existing AMBA driver without any >> changes matching the IDs partially. >> > I did not miss your last mail. Let me try to reply in even simpler > terms... PID[0,3] identification is fine grained enough. If/when we > have some platform with non-ARM MHU, we could catch that in this > driver. Most likely even then the reg-map won't change drastically, if > at all. So we could ignore the PID4, otherwise we check PID4 in this > driver and adapt the behavior. I am just looking for ARM MHU implementation, not any non-ARM MHU. If look at the MHU spec[1], excerpts below: PID_4 Register [31:8] - Reserved. Read as zero. [7:4] SIZE Indicates the log2 of the number of 4KB blocks that the interface occupies. Set to 0x0. [3:0] DES_2 JEP106 continuation code that identifies the designer. Set to 0x4 for ARM. PID_1 Register [31:8] - Reserved. Read as zero. [7:4] DES_0 Bits [3:0] of the JEP Identity. Set to 0xB for ARM. [3:0] PART_1 Bits [11:8] of the part number. Set to 0x0. Now by skipping PID4 DES_2 part, you are not comparing the designer correctly. It may be OK to fix in the driver as you mentioned. Not sure if it's better to fix AMBA, or just don't use AMBA for IP with JEP106 code. I don't have any strong opinion there. Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0515b/CHDBFACE.html#id5278622 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/03/15 13:19, Jassi Brar wrote: > On Wed, Mar 18, 2015 at 3:55 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >>> +static int mhu_send_data(struct mbox_chan *chan, void *data) >>> +{ >>> + struct mhu_link *mlink = chan->con_priv; >>> + u32 *arg = data; >> >> Arnd doesn't like this and had suggestions in some other thread. >> > No, Arnd suggested doing it this way. And another platform's driver > was made to do this way. > IIUC he suggested that it's better to add another interface/API to pass fixed-length something like inline int mbox_send_message_u32(struct mbox_chan *chan, u32 msg) { mbox_send_message(chan, &msg, sizeof(msg)); } and add a length argument to the existing mbox_send_message like: int mbox_send_message(struct mbox_chan *chan, void *mssg, int length) Am I missing something here ? >>> + writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS); >>> + >>> + return 0; >>> +} >>> + >>> +static int mhu_startup(struct mbox_chan *chan) >>> +{ >>> + struct mhu_link *mlink = chan->con_priv; >>> + u32 val; >>> + int ret; >>> + >>> + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); >>> + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); >>> + >>> + ret = request_irq(mlink->irq, mhu_rx_interrupt, >>> + IRQF_SHARED, "mhu_link", chan); >> >> >> Any reason we can't move this to probe and have {en,dis}able_irq here if >> needed. I has seen it was too heavy to have these especially when >> sending small packets. >> > I see you used to do memcpy in irq-handler > https://git.linaro.org/landing-teams/working/arm/kernel.git/blob/HEAD:/drivers/mailbox/arm_mhu.c > perhaps you were using your old driver? > That driver is too old and long abandoned. It mixes up the protocol details and was written when mailbox f/w was still under discussion. So you can forget that, it's out of scope of this discussion. > If you use this new driver, and send packets so often that > request-release irq has effect, maybe should hold the mailbox > reference for lifetime. I remember suggesting you that already and I > remember you said that's how it was. > Ah right, I keep getting confused that ops->startup is called from mbox_send_message for no reason, sorry for the noise. However, I found threaded_irq is much better for large packets. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/03/15 11:01, Vincent Yang wrote: > From: Jassi Brar <jaswinder.singh@linaro.org> > > Add driver for the ARM Primecell Message-Handling-Unit(MHU) controller. > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > Signed-off-by: Andy Green <andy.green@linaro.org> > Signed-off-by: Vincent Yang <vincent.yang@socionext.com> > Signed-off-by: Tetsuya Nuriya <nuriya.tetsuya@socionext.com> > --- > .../devicetree/bindings/mailbox/arm-mhu.txt | 43 +++++ > drivers/mailbox/Kconfig | 9 + > drivers/mailbox/Makefile | 2 + > drivers/mailbox/arm_mhu.c | 195 +++++++++++++++++++++ > 4 files changed, 249 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt > create mode 100644 drivers/mailbox/arm_mhu.c > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > new file mode 100644 > index 0000000..4971f03 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > @@ -0,0 +1,43 @@ > +ARM MHU Mailbox Driver > +====================== > + > +The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has > +3 independent channels/links to communicate with remote processor(s). > + MHU links are hardwired on a platform. A link raises interrupt for any > +received data. However, there is no specified way of knowing if the sent > +data has been read by the remote. This driver assumes the sender polls > +STAT register and the remote clears it after having read the data. > +The last channel is specified to be a 'Secure' resource, hence can't be > +used by Linux running NS. > + > +Mailbox Device Node: > +==================== > + > +Required properties: > +-------------------- > +- compatible: Shall be "arm,mhu" & "arm,primecell" > +- reg: Contains the mailbox register address range (base > + address and length) > +- #mbox-cells Shall be 1 - the index of the channel needed. > +- interrupts: Contains the interrupt information corresponding to > + each of the 3 links of MHU. > + I tried using this driver and found that AMBA driver expects apb_clk without which probe fails. Though your example have it, it's not explicit from the binding. Also AMBA binding expects the primecell id in the binding. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 26, 2015 at 11:43:21AM +0000, Sudeep Holla wrote: > I tried using this driver and found that AMBA driver expects apb_clk > without which probe fails. Though your example have it, it's not > explicit from the binding. Also AMBA binding expects the primecell id in > the binding. That's optional: Optional properties: - arm,primecell-periphid : Value to override the h/w value with It's only required when the hardware doesn't provide the correct value.
On 26/03/15 11:49, Russell King - ARM Linux wrote: > On Thu, Mar 26, 2015 at 11:43:21AM +0000, Sudeep Holla wrote: >> I tried using this driver and found that AMBA driver expects apb_clk >> without which probe fails. Though your example have it, it's not >> explicit from the binding. Also AMBA binding expects the primecell id in >> the binding. > > That's optional: > > Optional properties: > > - arm,primecell-periphid : Value to override the h/w value with > > It's only required when the hardware doesn't provide the correct value. > Ah sorry my bad, I meant the compatible(somehow left this word at the end of earlier email - it should have been" .. AMBA binding expects the primecell id in the binding compatible") - compatible : should be a specific name for the peripheral and "arm,primecell". The specific name will match the ARM engineering name for the logic block in the form: "arm,pl???" So I wanted to make sure either we follow that here(unlikely as ARM is not publishing this as standard primecell yet) or specify that it's exception here. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt new file mode 100644 index 0000000..4971f03 --- /dev/null +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt @@ -0,0 +1,43 @@ +ARM MHU Mailbox Driver +====================== + +The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has +3 independent channels/links to communicate with remote processor(s). + MHU links are hardwired on a platform. A link raises interrupt for any +received data. However, there is no specified way of knowing if the sent +data has been read by the remote. This driver assumes the sender polls +STAT register and the remote clears it after having read the data. +The last channel is specified to be a 'Secure' resource, hence can't be +used by Linux running NS. + +Mailbox Device Node: +==================== + +Required properties: +-------------------- +- compatible: Shall be "arm,mhu" & "arm,primecell" +- reg: Contains the mailbox register address range (base + address and length) +- #mbox-cells Shall be 1 - the index of the channel needed. +- interrupts: Contains the interrupt information corresponding to + each of the 3 links of MHU. + +Example: +-------- + + mhu: mailbox@2b1f0000 { + #mbox-cells = <1>; + compatible = "arm,mhu", "arm,primecell"; + reg = <0 0x2b1f0000 0x1000>; + interrupts = <0 36 4>, /* LP-NonSecure */ + <0 35 4>, /* HP-NonSecure */ + <0 37 4>; /* Secure */ + clocks = <&clock 0 2 1>; + clock-names = "apb_pclk"; + }; + + mhu_client: scb@2e000000 { + compatible = "fujitsu,mb86s70-scb-1.0"; + reg = <0 0x2e000000 0x4000>; + mboxes = <&mhu 1>; /* HP-NonSecure */ + }; diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 84325f2..84b0a2d 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -6,6 +6,15 @@ menuconfig MAILBOX signals. Say Y if your platform supports hardware mailboxes. if MAILBOX + +config ARM_MHU + tristate "ARM MHU Mailbox" + depends on ARM_AMBA + help + Say Y here if you want to build the ARM MHU controller driver. + The controller has 3 mailbox channels, the last of which can be + used in Secure mode only. + config PL320_MBOX bool "ARM PL320 Mailbox" depends on ARM_AMBA diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 2e79231..b18201e 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -2,6 +2,8 @@ obj-$(CONFIG_MAILBOX) += mailbox.o +obj-$(CONFIG_ARM_MHU) += arm_mhu.o + obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c new file mode 100644 index 0000000..ac693c6 --- /dev/null +++ b/drivers/mailbox/arm_mhu.c @@ -0,0 +1,195 @@ +/* + * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd. + * Copyright (C) 2015 Linaro Ltd. + * Author: Jassi Brar <jaswinder.singh@linaro.org> + * + * 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/module.h> +#include <linux/amba/bus.h> +#include <linux/mailbox_controller.h> + +#define INTR_STAT_OFS 0x0 +#define INTR_SET_OFS 0x8 +#define INTR_CLR_OFS 0x10 + +#define MHU_LP_OFFSET 0x0 +#define MHU_HP_OFFSET 0x20 +#define MHU_SEC_OFFSET 0x200 +#define TX_REG_OFFSET 0x100 + +#define MHU_CHANS 3 + +struct mhu_link { + unsigned irq; + void __iomem *tx_reg; + void __iomem *rx_reg; +}; + +struct arm_mhu { + void __iomem *base; + struct mhu_link mlink[MHU_CHANS]; + struct mbox_chan chan[MHU_CHANS]; + struct mbox_controller mbox; +}; + +static irqreturn_t mhu_rx_interrupt(int irq, void *p) +{ + struct mbox_chan *chan = p; + struct mhu_link *mlink = chan->con_priv; + u32 val; + + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); + if (!val) + return IRQ_NONE; + + mbox_chan_received_data(chan, (void *)&val); + + 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 = chan->con_priv; + u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); + + return (val == 0); +} + +static int mhu_send_data(struct mbox_chan *chan, void *data) +{ + struct mhu_link *mlink = chan->con_priv; + u32 *arg = data; + + writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS); + + return 0; +} + +static int mhu_startup(struct mbox_chan *chan) +{ + struct mhu_link *mlink = chan->con_priv; + u32 val; + int ret; + + val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS); + writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS); + + ret = request_irq(mlink->irq, mhu_rx_interrupt, + IRQF_SHARED, "mhu_link", chan); + if (ret) { + dev_err(chan->mbox->dev, + "Unable to aquire IRQ %d\n", mlink->irq); + return ret; + } + + return 0; +} + +static void mhu_shutdown(struct mbox_chan *chan) +{ + struct mhu_link *mlink = chan->con_priv; + + 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 mhu_probe(struct amba_device *adev, const struct amba_id *id) +{ + int i, err; + struct arm_mhu *mhu; + struct device *dev = &adev->dev; + int mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET}; + + /* Allocate memory for device */ + mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL); + if (!mhu) + return -ENOMEM; + + mhu->base = devm_ioremap_resource(dev, &adev->res); + if (IS_ERR(mhu->base)) { + dev_err(dev, "ioremap failed\n"); + return PTR_ERR(mhu->base); + } + + for (i = 0; i < MHU_CHANS; i++) { + mhu->chan[i].con_priv = &mhu->mlink[i]; + mhu->mlink[i].irq = adev->irq[i]; + mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i]; + mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET; + } + + mhu->mbox.dev = dev; + mhu->mbox.chans = &mhu->chan[0]; + mhu->mbox.num_chans = MHU_CHANS; + mhu->mbox.ops = &mhu_ops; + mhu->mbox.txdone_irq = false; + mhu->mbox.txdone_poll = true; + mhu->mbox.txpoll_period = 10; + + amba_set_drvdata(adev, mhu); + + err = mbox_controller_register(&mhu->mbox); + if (err) { + dev_err(dev, "Failed to register mailboxes %d\n", err); + return err; + } + + dev_info(dev, "ARM MHU Mailbox registered\n"); + return 0; +} + +static int mhu_remove(struct amba_device *adev) +{ + struct arm_mhu *mhu = amba_get_drvdata(adev); + + mbox_controller_unregister(&mhu->mbox); + + return 0; +} + +static struct amba_id mhu_ids[] = { + { + .id = 0x1bb098, + .mask = 0xffffff, + }, + { 0, 0 }, +}; +MODULE_DEVICE_TABLE(amba, mhu_ids); + +static struct amba_driver arm_mhu_driver = { + .drv = { + .name = "mhu", + }, + .id_table = mhu_ids, + .probe = mhu_probe, + .remove = mhu_remove, +}; +module_amba_driver(arm_mhu_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ARM MHU Driver"); +MODULE_AUTHOR("Jassi Brar <jassisinghbrar@gmail.com>");