2015-04-22 22:18:47

by Feng Kan

[permalink] [raw]
Subject: [PATCH V2 0/4] APM X-Gene Mailbox driver

This adds the mailbox driver for the APM X-Gene SoC

V2 change
- Add ACPI support
- use defines for reg offset

Feng Kan (4):
mailbox: add support for APM X-Gene platform mailbox driver
mailbox: xgene: add ACPI support for X-Gene mailbox driver
Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts
documentation
arm64: dts: mailbox device tree node for APM X-Gene platform.

.../bindings/mailbox/xgene-slimpro-mailbox.txt | 34 +++
arch/arm64/boot/dts/apm/apm-storm.dtsi | 14 ++
drivers/mailbox/Kconfig | 10 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-xgene-slimpro.c | 271 +++++++++++++++++++++
5 files changed, 331 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

--
1.9.1


2015-04-22 22:19:44

by Feng Kan

[permalink] [raw]
Subject: [PATCH V2 1/4] mailbox: add support for APM X-Gene platform mailbox driver

Add support for APM X-Gene platform mailbox driver.

Signed-off-by: Feng Kan <[email protected]>
---
drivers/mailbox/Kconfig | 10 ++
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-xgene-slimpro.c | 261 ++++++++++++++++++++++++++++++++
3 files changed, 273 insertions(+)
create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 84b0a2d..64120cc 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -60,4 +60,14 @@ config ALTERA_MBOX
An implementation of the Altera Mailbox soft core. It is used
to send message between processors. Say Y here if you want to use the
Altera mailbox support.
+
+config XGENE_SLIMPRO_MBOX
+ tristate "APM SoC X-Gene SLIMpro Mailbox Controller"
+ depends on ARCH_XGENE
+ help
+ An implementation of the APM X-Gene Interprocessor Communication
+ Mailbox (IPCM) between the ARM 64-bit cores and SLIMpro controller.
+ It is used to send short messages between ARM64-bit cores and
+ the SLIMpro Management Engine, primarily for PM. Say Y here if you
+ want to use the APM X-Gene SLIMpro IPCM support.
endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index b18201e..1e0bed5 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o
obj-$(CONFIG_PCC) += pcc.o

obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o
+
+obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
diff --git a/drivers/mailbox/mailbox-xgene-slimpro.c b/drivers/mailbox/mailbox-xgene-slimpro.c
new file mode 100644
index 0000000..49370e5
--- /dev/null
+++ b/drivers/mailbox/mailbox-xgene-slimpro.c
@@ -0,0 +1,261 @@
+/*
+ * APM X-Gene SLIMpro MailBox Driver
+ *
+ * Copyright (c) 2014, Applied Micro Circuits Corporation
+ * Author: Feng Kan [email protected]
+ *
+ * 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; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define MBOX_CON_NAME "slimpro-mbox"
+#define MBOX_REG_SET_OFFSET 0x1000
+#define MBOX_CNT 8
+#define MBOX_STATUS_AVAIL_MASK BIT(16)
+#define MBOX_STATUS_ACK_MASK BIT(0)
+
+/* Configuration and Status Registers */
+#define REG_DB_IN 0x00
+#define REG_DB_DIN0 0x04
+#define REG_DB_DIN1 0x08
+#define REG_DB_OUT 0x10
+#define REG_DB_DOUT0 0x14
+#define REG_DB_DOUT1 0x18
+#define REG_DB_STAT 0x20
+#define REG_DB_STATMASK 0x24
+
+struct slimpro_mbox_chan {
+ struct device *dev;
+ struct mbox_chan *chan;
+ void __iomem *reg;
+ int id;
+ int irq;
+ u32 rx_msg[3];
+};
+
+struct slimpro_mbox {
+ struct mbox_controller mb_ctrl;
+ struct slimpro_mbox_chan mc[MBOX_CNT];
+ struct mbox_chan chans[MBOX_CNT];
+};
+
+static inline void mb_chan_send_msg(struct slimpro_mbox_chan *mb_chan, u32 *msg)
+{
+ writel(msg[1], mb_chan->reg + REG_DB_DOUT0);
+ writel(msg[2], mb_chan->reg + REG_DB_DOUT1);
+ writel(msg[0], mb_chan->reg + REG_DB_OUT);
+}
+
+static inline void mb_chan_recv_msg(struct slimpro_mbox_chan *mb_chan)
+{
+ mb_chan->rx_msg[1] = readl(mb_chan->reg + REG_DB_DIN0);
+ mb_chan->rx_msg[2] = readl(mb_chan->reg + REG_DB_DIN1);
+ mb_chan->rx_msg[0] = readl(mb_chan->reg + REG_DB_IN);
+}
+
+static void mb_chan_enable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
+
+ val &= ~mask;
+
+ writel(val, mb_chan->reg + REG_DB_STATMASK);
+}
+
+static void mb_chan_disable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
+
+ val |= mask;
+
+ writel(val, mb_chan->reg + REG_DB_STATMASK);
+}
+
+static int mb_chan_status_ack(struct slimpro_mbox_chan *mb_chan)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STAT);
+
+ if (val & MBOX_STATUS_ACK_MASK) {
+ writel(MBOX_STATUS_ACK_MASK, mb_chan->reg + REG_DB_STAT);
+ return 1;
+ }
+ return 0;
+}
+
+static int mb_chan_status_avail(struct slimpro_mbox_chan *mb_chan)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STAT);
+
+ if (val & MBOX_STATUS_AVAIL_MASK) {
+ mb_chan_recv_msg(mb_chan);
+ writel(MBOX_STATUS_AVAIL_MASK, mb_chan->reg + REG_DB_STAT);
+ return 1;
+ }
+ return 0;
+}
+
+static irqreturn_t slimpro_mbox_irq(int irq, void *id)
+{
+ struct slimpro_mbox_chan *mb_chan = id;
+
+ if (mb_chan_status_ack(mb_chan))
+ mbox_chan_txdone(mb_chan->chan, 0);
+
+ if (mb_chan_status_avail(mb_chan))
+ mbox_chan_received_data(mb_chan->chan, mb_chan->rx_msg);
+
+ return IRQ_HANDLED;
+}
+
+static int slimpro_mbox_send_data(struct mbox_chan *chan, void *msg)
+{
+ struct slimpro_mbox_chan *mb_chan =
+ (struct slimpro_mbox_chan *)chan->con_priv;
+
+ mb_chan_send_msg(mb_chan, msg);
+ return 0;
+}
+
+static int slimpro_mbox_startup(struct mbox_chan *chan)
+{
+ struct slimpro_mbox_chan *mb_chan =
+ (struct slimpro_mbox_chan *)chan->con_priv;
+ int rc;
+
+ rc = devm_request_irq(mb_chan->dev, mb_chan->irq, slimpro_mbox_irq, 0,
+ MBOX_CON_NAME, mb_chan);
+ if (unlikely(rc)) {
+ dev_err(mb_chan->dev, "failed to register mailbox interrupt %d\n",
+ mb_chan->irq);
+ return rc;
+ }
+
+ /* Enable HW interrupt */
+ writel(MBOX_STATUS_ACK_MASK | MBOX_STATUS_AVAIL_MASK,
+ mb_chan->reg + REG_DB_STAT);
+ mb_chan_enable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+ MBOX_STATUS_AVAIL_MASK);
+ return 0;
+}
+
+static void slimpro_mbox_shutdown(struct mbox_chan *chan)
+{
+ struct slimpro_mbox_chan *mb_chan =
+ (struct slimpro_mbox_chan *)chan->con_priv;
+
+ mb_chan_disable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+ MBOX_STATUS_AVAIL_MASK);
+ devm_free_irq(mb_chan->dev, mb_chan->irq, mb_chan);
+}
+
+static struct mbox_chan_ops slimpro_mbox_ops = {
+ .send_data = slimpro_mbox_send_data,
+ .startup = slimpro_mbox_startup,
+ .shutdown = slimpro_mbox_shutdown,
+};
+
+static int __init slimpro_mbox_probe(struct platform_device *pdev)
+{
+ struct slimpro_mbox *ctx;
+ struct resource *regs;
+ void __iomem *mb_base;
+ int rc;
+ int i;
+
+ ctx = devm_kzalloc(&pdev->dev, sizeof(struct slimpro_mbox), GFP_KERNEL);
+ if (IS_ERR(ctx))
+ return PTR_ERR(ctx);
+
+ platform_set_drvdata(pdev, ctx);
+
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ mb_base = devm_ioremap(&pdev->dev, regs->start, resource_size(regs));
+ if (IS_ERR(mb_base))
+ return PTR_ERR(mb_base);
+
+ /* Setup mailbox links */
+ for (i = 0; i < MBOX_CNT; i++) {
+ ctx->mc[i].irq = platform_get_irq(pdev, i);
+ if (ctx->mc[i].irq < 0) {
+ dev_err(&pdev->dev, "no IRQ at index %d\n",
+ ctx->mc[i].irq);
+ return -ENODEV;
+ }
+
+ ctx->mc[i].dev = &pdev->dev;
+ ctx->mc[i].reg = mb_base + i * MBOX_REG_SET_OFFSET;
+ ctx->mc[i].id = i;
+ ctx->mc[i].chan = &ctx->chans[i];
+ ctx->chans[i].con_priv = &ctx->mc[i];
+ }
+
+ /* Setup mailbox controller */
+ ctx->mb_ctrl.dev = &pdev->dev;
+ ctx->mb_ctrl.chans = ctx->chans;
+ ctx->mb_ctrl.txdone_irq = true;
+ ctx->mb_ctrl.ops = &slimpro_mbox_ops;
+ ctx->mb_ctrl.num_chans = MBOX_CNT;
+
+ rc = mbox_controller_register(&ctx->mb_ctrl);
+ if (rc) {
+ dev_err(&pdev->dev,
+ "APM X-Gene SLIMpro MailBox register failed:%d\n", rc);
+ return rc;
+ }
+
+ dev_info(&pdev->dev, "APM X-Gene SLIMpro MailBox registered\n");
+ return 0;
+}
+
+static int slimpro_mbox_remove(struct platform_device *pdev)
+{
+ struct slimpro_mbox *smb = platform_get_drvdata(pdev);
+
+ mbox_controller_unregister(&smb->mb_ctrl);
+ return 0;
+}
+
+static const struct of_device_id slimpro_of_match[] = {
+ {.compatible = "apm,xgene-slimpro-mbox" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, slimpro_of_match);
+
+static struct platform_driver slimpro_mbox_driver = {
+ .probe = slimpro_mbox_probe,
+ .remove = slimpro_mbox_remove,
+ .driver = {
+ .name = "xgene-slimpro-mbox",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(slimpro_of_match),
+ },
+};
+
+static int __init slimpro_mbox_init(void)
+{
+ return platform_driver_register(&slimpro_mbox_driver);
+}
+
+subsys_initcall(slimpro_mbox_init);
+
+MODULE_DESCRIPTION("APM X-Gene SLIMpro Mailbox Driver");
+MODULE_LICENSE("GPL");
--
1.9.1

2015-04-22 22:19:42

by Feng Kan

[permalink] [raw]
Subject: [PATCH V2 2/4] mailbox: xgene: add ACPI support for X-Gene mailbox driver

Add ACPI support for APM X-Gene mailbox driver.

Signed-off-by: Feng Kan <[email protected]>
---
drivers/mailbox/mailbox-xgene-slimpro.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/mailbox/mailbox-xgene-slimpro.c b/drivers/mailbox/mailbox-xgene-slimpro.c
index 49370e5..890f95b 100644
--- a/drivers/mailbox/mailbox-xgene-slimpro.c
+++ b/drivers/mailbox/mailbox-xgene-slimpro.c
@@ -18,6 +18,7 @@
* along with this program; if not, see <http://www.gnu.org/licenses/>.
*
*/
+#include <linux/acpi.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/io.h>
@@ -240,6 +241,14 @@ static const struct of_device_id slimpro_of_match[] = {
};
MODULE_DEVICE_TABLE(of, slimpro_of_match);

+#ifdef CONFIG_ACPI
+static const struct acpi_device_id slimpro_acpi_ids[] = {
+ {"APMC0D01", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, slimpro_acpi_ids);
+#endif
+
static struct platform_driver slimpro_mbox_driver = {
.probe = slimpro_mbox_probe,
.remove = slimpro_mbox_remove,
@@ -247,6 +256,7 @@ static struct platform_driver slimpro_mbox_driver = {
.name = "xgene-slimpro-mbox",
.owner = THIS_MODULE,
.of_match_table = of_match_ptr(slimpro_of_match),
+ .acpi_match_table = ACPI_PTR(slimpro_acpi_ids)
},
};

--
1.9.1

2015-04-22 22:18:58

by Feng Kan

[permalink] [raw]
Subject: [PATCH V2 3/4] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

This adds the APM X-Gene SLIMpro mailbox device tree node documentation.

Signed-off-by: Feng Kan <[email protected]>
---
.../bindings/mailbox/xgene-slimpro-mailbox.txt | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
new file mode 100644
index 0000000..d02a3d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
@@ -0,0 +1,34 @@
+The APM X-Gene SLIMpro mailbox is used to communicate messages between
+the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
+interrupt based door bell mechanism and can exchange simple messages using the
+internal registers.
+
+There are total of 7 interrupts in this mailbox. Each used for an individual
+door bell (or mailbox channel).
+
+Required properties:
+- compatible: Should be as "apm, xgene-slimpro-mbox".
+
+- reg: Contain the mailbox register address range.
+
+- interrupts: 7 interrupts must start from 0 to 6, interrupt 0 define the
+ the interrupt for mailbox channel 0 and interrupt 1 for
+ mailbox channel 1 and so likewise for the reminder.
+
+- #mbox-cells: only one to specify the mailbox channel number.
+
+Example:
+
+Mailbox Node:
+ slimpro-mbox: slimpro-mbox@10540000 {
+ compatible = "apm,xgene-slimpro-mbox";
+ reg = <0x0 0x10540000 0x0 0xa000>;
+ #mbox-cells = <1>;
+ interrupts = <0x0 0x0 0x4>,
+ <0x0 0x1 0x4>,
+ <0x0 0x2 0x4>,
+ <0x0 0x3 0x4>,
+ <0x0 0x4 0x4>,
+ <0x0 0x5 0x4>,
+ <0x0 0x6 0x4>,
+ };
--
1.9.1

2015-04-22 22:19:00

by Feng Kan

[permalink] [raw]
Subject: [PATCH V2 4/4] arm64: dts: mailbox device tree node for APM X-Gene platform.

Mailbox device tree node for APM X-Gene platform.

Signed-off-by: Feng Kan <[email protected]>
---
arch/arm64/boot/dts/apm/apm-storm.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index c8d3e0e..92e7b6b 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -489,6 +489,20 @@
clocks = <&pcie4clk 0>;
};

+ mailbox: slimpro-mbox@10540000 {
+ compatible = "apm,xgene-slimpro-mbox";
+ reg = <0x0 0x10540000 0x0 0xa000>;
+ #mbox-cells = <1>;
+ interrupts = <0x0 0x0 0x4>,
+ <0x0 0x1 0x4>,
+ <0x0 0x2 0x4>,
+ <0x0 0x3 0x4>,
+ <0x0 0x4 0x4>,
+ <0x0 0x5 0x4>,
+ <0x0 0x6 0x4>,
+ <0x0 0x7 0x4>;
+ };
+
serial0: serial@1c020000 {
status = "disabled";
device_type = "serial";
--
1.9.1

2015-04-24 08:25:22

by Paul Bolle

[permalink] [raw]
Subject: Re: [PATCH V2 1/4] mailbox: add support for APM X-Gene platform mailbox driver

On Wed, 2015-04-22 at 15:18 -0700, Feng Kan wrote:
> --- /dev/null
> +++ b/drivers/mailbox/mailbox-xgene-slimpro.c

> +static struct platform_driver slimpro_mbox_driver = {
> + .probe = slimpro_mbox_probe,
> + .remove = slimpro_mbox_remove,
> + .driver = {
> + .name = "xgene-slimpro-mbox",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(slimpro_of_match),
> + },
> +};
> +
> +static int __init slimpro_mbox_init(void)
> +{
> + return platform_driver_register(&slimpro_mbox_driver);
> +}
> +
> +subsys_initcall(slimpro_mbox_init);

After a quick scan of this driver you'd expect something like
static void __exit slimpro_mbox_exit(void)
{
platform_driver_unregister(&slimpro_mbox_driver);
}
module_exit(slimpro_mbox_exit);

too here. At least, I do. Is there some non-obvious (to me) reason this
driver can't be unloaded if it is built as a module?

> +MODULE_DESCRIPTION("APM X-Gene SLIMpro Mailbox Driver");
> +MODULE_LICENSE("GPL");

Thanks,


Paul Bolle

2015-11-09 18:08:06

by Duc Dang

[permalink] [raw]
Subject: [PATCH v3 0/3] mailbox: Add APM X-Gene platform mailbox driver

APM X-Gene SoC has a mailbox controller that provides
communication mechanism for X-Gene Arm64 cores to communicate
with X-Gene SoC's Cortex M3 (SLIMpro) processor.

X-Gene mailbox controller provides 8 mailbox channels, with
each channel has a dedicated interrupt line.

Changes since v2:
- Rebase Feng's patch set over v4.3-rc5
- Remove uneccessary 'inline' in function definition
- Use module_platform_driver instead of subsys_initcall
- Minor coding stype clean up

Changes since v1:
- Add ACPI support
- Use defines for reg offset

.../bindings/mailbox/xgene-slimpro-mailbox.txt | 34 +++
arch/arm64/boot/dts/apm/apm-storm.dtsi | 14 ++
drivers/mailbox/Kconfig | 9 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-xgene-slimpro.c | 264 +++++++++++++++++++++
5 files changed, 323 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

--
1.9.1

2015-11-09 18:08:11

by Duc Dang

[permalink] [raw]
Subject: [PATCH v3 1/3] mailbox: Add support for APM X-Gene platform mailbox driver

X-Gene mailbox controller provides 8 mailbox channels, with
each channel has a dedicated interrupt line.

[dhdang: rebase over 4.3-rc5, some minor changes to
address comment in v2 patch set]
Signed-off-by: Feng Kan <[email protected]>
Signed-off-by: Duc Dang <[email protected]>
---
drivers/mailbox/Kconfig | 9 ++
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-xgene-slimpro.c | 264 ++++++++++++++++++++++++++++++++
3 files changed, 275 insertions(+)
create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index bbec500..ae37d39 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -71,4 +71,13 @@ config BCM2835_MBOX
the services of the Videocore. Say Y here if you want to use the
BCM2835 Mailbox.

+config XGENE_SLIMPRO_MBOX
+ tristate "APM SoC X-Gene SLIMpro Mailbox Controller"
+ depends on ARCH_XGENE
+ help
+ An implementation of the APM X-Gene Interprocessor Communication
+ Mailbox (IPCM) between the ARM 64-bit cores and SLIMpro controller.
+ It is used to send short messages between ARM64-bit cores and
+ the SLIMpro Management Engine, primarily for PM. Say Y here if you
+ want to use the APM X-Gene SLIMpro IPCM support.
endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 8e6d822..6a78df7 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCC) += pcc.o
obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o

obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o
+
+obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
diff --git a/drivers/mailbox/mailbox-xgene-slimpro.c b/drivers/mailbox/mailbox-xgene-slimpro.c
new file mode 100644
index 0000000..4753353
--- /dev/null
+++ b/drivers/mailbox/mailbox-xgene-slimpro.c
@@ -0,0 +1,264 @@
+/*
+ * APM X-Gene SLIMpro MailBox Driver
+ *
+ * Copyright (c) 2015, Applied Micro Circuits Corporation
+ * Author: Feng Kan [email protected]
+ *
+ * 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; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define MBOX_CON_NAME "slimpro-mbox"
+#define MBOX_REG_SET_OFFSET 0x1000
+#define MBOX_CNT 8
+#define MBOX_STATUS_AVAIL_MASK BIT(16)
+#define MBOX_STATUS_ACK_MASK BIT(0)
+
+/* Configuration and Status Registers */
+#define REG_DB_IN 0x00
+#define REG_DB_DIN0 0x04
+#define REG_DB_DIN1 0x08
+#define REG_DB_OUT 0x10
+#define REG_DB_DOUT0 0x14
+#define REG_DB_DOUT1 0x18
+#define REG_DB_STAT 0x20
+#define REG_DB_STATMASK 0x24
+
+struct slimpro_mbox_chan {
+ struct device *dev;
+ struct mbox_chan *chan;
+ void __iomem *reg;
+ int id;
+ int irq;
+ u32 rx_msg[3];
+};
+
+struct slimpro_mbox {
+ struct mbox_controller mb_ctrl;
+ struct slimpro_mbox_chan mc[MBOX_CNT];
+ struct mbox_chan chans[MBOX_CNT];
+};
+
+static void mb_chan_send_msg(struct slimpro_mbox_chan *mb_chan, u32 *msg)
+{
+ writel(msg[1], mb_chan->reg + REG_DB_DOUT0);
+ writel(msg[2], mb_chan->reg + REG_DB_DOUT1);
+ writel(msg[0], mb_chan->reg + REG_DB_OUT);
+}
+
+static void mb_chan_recv_msg(struct slimpro_mbox_chan *mb_chan)
+{
+ mb_chan->rx_msg[1] = readl(mb_chan->reg + REG_DB_DIN0);
+ mb_chan->rx_msg[2] = readl(mb_chan->reg + REG_DB_DIN1);
+ mb_chan->rx_msg[0] = readl(mb_chan->reg + REG_DB_IN);
+}
+
+static void mb_chan_enable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
+
+ val &= ~mask;
+ writel(val, mb_chan->reg + REG_DB_STATMASK);
+}
+
+static void mb_chan_disable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
+
+ val |= mask;
+ writel(val, mb_chan->reg + REG_DB_STATMASK);
+}
+
+static int mb_chan_status_ack(struct slimpro_mbox_chan *mb_chan)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STAT);
+
+ if (val & MBOX_STATUS_ACK_MASK) {
+ writel(MBOX_STATUS_ACK_MASK, mb_chan->reg + REG_DB_STAT);
+ return 1;
+ }
+ return 0;
+}
+
+static int mb_chan_status_avail(struct slimpro_mbox_chan *mb_chan)
+{
+ u32 val = readl(mb_chan->reg + REG_DB_STAT);
+
+ if (val & MBOX_STATUS_AVAIL_MASK) {
+ mb_chan_recv_msg(mb_chan);
+ writel(MBOX_STATUS_AVAIL_MASK, mb_chan->reg + REG_DB_STAT);
+ return 1;
+ }
+ return 0;
+}
+
+static irqreturn_t slimpro_mbox_irq(int irq, void *id)
+{
+ struct slimpro_mbox_chan *mb_chan = id;
+
+ if (mb_chan_status_ack(mb_chan))
+ mbox_chan_txdone(mb_chan->chan, 0);
+
+ if (mb_chan_status_avail(mb_chan))
+ mbox_chan_received_data(mb_chan->chan, mb_chan->rx_msg);
+
+ return IRQ_HANDLED;
+}
+
+static int slimpro_mbox_send_data(struct mbox_chan *chan, void *msg)
+{
+ struct slimpro_mbox_chan *mb_chan =
+ (struct slimpro_mbox_chan *)chan->con_priv;
+
+ mb_chan_send_msg(mb_chan, msg);
+ return 0;
+}
+
+static int slimpro_mbox_startup(struct mbox_chan *chan)
+{
+ struct slimpro_mbox_chan *mb_chan =
+ (struct slimpro_mbox_chan *)chan->con_priv;
+ int rc;
+
+ rc = devm_request_irq(mb_chan->dev, mb_chan->irq, slimpro_mbox_irq, 0,
+ MBOX_CON_NAME, mb_chan);
+ if (unlikely(rc)) {
+ dev_err(mb_chan->dev, "failed to register mailbox interrupt %d\n",
+ mb_chan->irq);
+ return rc;
+ }
+
+ /* Enable HW interrupt */
+ writel(MBOX_STATUS_ACK_MASK | MBOX_STATUS_AVAIL_MASK,
+ mb_chan->reg + REG_DB_STAT);
+ mb_chan_enable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+ MBOX_STATUS_AVAIL_MASK);
+ return 0;
+}
+
+static void slimpro_mbox_shutdown(struct mbox_chan *chan)
+{
+ struct slimpro_mbox_chan *mb_chan =
+ (struct slimpro_mbox_chan *)chan->con_priv;
+
+ mb_chan_disable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+ MBOX_STATUS_AVAIL_MASK);
+ devm_free_irq(mb_chan->dev, mb_chan->irq, mb_chan);
+}
+
+static struct mbox_chan_ops slimpro_mbox_ops = {
+ .send_data = slimpro_mbox_send_data,
+ .startup = slimpro_mbox_startup,
+ .shutdown = slimpro_mbox_shutdown,
+};
+
+static int __init slimpro_mbox_probe(struct platform_device *pdev)
+{
+ struct slimpro_mbox *ctx;
+ struct resource *regs;
+ void __iomem *mb_base;
+ int rc;
+ int i;
+
+ ctx = devm_kzalloc(&pdev->dev, sizeof(struct slimpro_mbox), GFP_KERNEL);
+ if (IS_ERR(ctx))
+ return PTR_ERR(ctx);
+
+ platform_set_drvdata(pdev, ctx);
+
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ mb_base = devm_ioremap(&pdev->dev, regs->start, resource_size(regs));
+ if (IS_ERR(mb_base))
+ return PTR_ERR(mb_base);
+
+ /* Setup mailbox links */
+ for (i = 0; i < MBOX_CNT; i++) {
+ ctx->mc[i].irq = platform_get_irq(pdev, i);
+ if (ctx->mc[i].irq < 0) {
+ dev_err(&pdev->dev, "no IRQ at index %d\n",
+ ctx->mc[i].irq);
+ return -ENODEV;
+ }
+
+ ctx->mc[i].dev = &pdev->dev;
+ ctx->mc[i].reg = mb_base + i * MBOX_REG_SET_OFFSET;
+ ctx->mc[i].id = i;
+ ctx->mc[i].chan = &ctx->chans[i];
+ ctx->chans[i].con_priv = &ctx->mc[i];
+ }
+
+ /* Setup mailbox controller */
+ ctx->mb_ctrl.dev = &pdev->dev;
+ ctx->mb_ctrl.chans = ctx->chans;
+ ctx->mb_ctrl.txdone_irq = true;
+ ctx->mb_ctrl.ops = &slimpro_mbox_ops;
+ ctx->mb_ctrl.num_chans = MBOX_CNT;
+
+ rc = mbox_controller_register(&ctx->mb_ctrl);
+ if (rc) {
+ dev_err(&pdev->dev,
+ "APM X-Gene SLIMpro MailBox register failed:%d\n", rc);
+ return rc;
+ }
+
+ dev_info(&pdev->dev, "APM X-Gene SLIMpro MailBox registered\n");
+ return 0;
+}
+
+static int slimpro_mbox_remove(struct platform_device *pdev)
+{
+ struct slimpro_mbox *smb = platform_get_drvdata(pdev);
+
+ mbox_controller_unregister(&smb->mb_ctrl);
+ return 0;
+}
+
+static const struct of_device_id slimpro_of_match[] = {
+ {.compatible = "apm,xgene-slimpro-mbox" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, slimpro_of_match);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id slimpro_acpi_ids[] = {
+ {"APMC0D01", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, slimpro_acpi_ids);
+#endif
+
+static struct platform_driver slimpro_mbox_driver = {
+ .probe = slimpro_mbox_probe,
+ .remove = slimpro_mbox_remove,
+ .driver = {
+ .name = "xgene-slimpro-mbox",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(slimpro_of_match),
+ .acpi_match_table = ACPI_PTR(slimpro_acpi_ids)
+ },
+};
+
+module_platform_driver(slimpro_mbox_driver);
+
+MODULE_DESCRIPTION("APM X-Gene SLIMpro Mailbox Driver");
+MODULE_LICENSE("GPL");
--
1.9.1

2015-11-09 18:08:43

by Duc Dang

[permalink] [raw]
Subject: [PATCH v3 2/3] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

This adds the APM X-Gene SLIMpro mailbox device tree
node documentation.

[dhdang: rebase over 4.3-rc5]
Signed-off-by: Feng Kan <[email protected]>
Signed-off-by: Duc Dang <[email protected]>
---
.../bindings/mailbox/xgene-slimpro-mailbox.txt | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
new file mode 100644
index 0000000..d02a3d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
@@ -0,0 +1,34 @@
+The APM X-Gene SLIMpro mailbox is used to communicate messages between
+the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
+interrupt based door bell mechanism and can exchange simple messages using the
+internal registers.
+
+There are total of 7 interrupts in this mailbox. Each used for an individual
+door bell (or mailbox channel).
+
+Required properties:
+- compatible: Should be as "apm, xgene-slimpro-mbox".
+
+- reg: Contain the mailbox register address range.
+
+- interrupts: 7 interrupts must start from 0 to 6, interrupt 0 define the
+ the interrupt for mailbox channel 0 and interrupt 1 for
+ mailbox channel 1 and so likewise for the reminder.
+
+- #mbox-cells: only one to specify the mailbox channel number.
+
+Example:
+
+Mailbox Node:
+ slimpro-mbox: slimpro-mbox@10540000 {
+ compatible = "apm,xgene-slimpro-mbox";
+ reg = <0x0 0x10540000 0x0 0xa000>;
+ #mbox-cells = <1>;
+ interrupts = <0x0 0x0 0x4>,
+ <0x0 0x1 0x4>,
+ <0x0 0x2 0x4>,
+ <0x0 0x3 0x4>,
+ <0x0 0x4 0x4>,
+ <0x0 0x5 0x4>,
+ <0x0 0x6 0x4>,
+ };
--
1.9.1

2015-11-09 18:08:26

by Duc Dang

[permalink] [raw]
Subject: [PATCH v3 3/3] arm64: dts: mailbox device tree node for APM X-Gene platform.

Mailbox device tree node for APM X-Gene platform.

[dhdang: rebase over 4.3-rc5]
Signed-off-by: Feng Kan <[email protected]>
Signed-off-by: Duc Dang <[email protected]>
---
arch/arm64/boot/dts/apm/apm-storm.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index d831bc2..b36e555 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -604,6 +604,20 @@
msi-parent = <&msi>;
};

+ mailbox: slimpro-mbox@10540000 {
+ compatible = "apm,xgene-slimpro-mbox";
+ reg = <0x0 0x10540000 0x0 0xa000>;
+ #mbox-cells = <1>;
+ interrupts = <0x0 0x0 0x4>,
+ <0x0 0x1 0x4>,
+ <0x0 0x2 0x4>,
+ <0x0 0x3 0x4>,
+ <0x0 0x4 0x4>,
+ <0x0 0x5 0x4>,
+ <0x0 0x6 0x4>,
+ <0x0 0x7 0x4>;
+ };
+
serial0: serial@1c020000 {
status = "disabled";
device_type = "serial";
--
1.9.1

2015-11-10 14:15:18

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

On Mon, Nov 09, 2015 at 10:05:44AM -0800, Duc Dang wrote:
> This adds the APM X-Gene SLIMpro mailbox device tree
> node documentation.
>
> [dhdang: rebase over 4.3-rc5]
> Signed-off-by: Feng Kan <[email protected]>
> Signed-off-by: Duc Dang <[email protected]>
> ---
> .../bindings/mailbox/xgene-slimpro-mailbox.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
>
> diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> new file mode 100644
> index 0000000..d02a3d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> @@ -0,0 +1,34 @@
> +The APM X-Gene SLIMpro mailbox is used to communicate messages between
> +the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
> +interrupt based door bell mechanism and can exchange simple messages using the
> +internal registers.
> +
> +There are total of 7 interrupts in this mailbox. Each used for an individual
> +door bell (or mailbox channel).
> +
> +Required properties:
> +- compatible: Should be as "apm, xgene-slimpro-mbox".
^
remove the space

> +
> +- reg: Contain the mailbox register address range.

s/Contain/Contains/

> +
> +- interrupts: 7 interrupts must start from 0 to 6, interrupt 0 define the
> + the interrupt for mailbox channel 0 and interrupt 1 for
> + mailbox channel 1 and so likewise for the reminder.

s/start/be/

> +
> +- #mbox-cells: only one to specify the mailbox channel number.
> +
> +Example:
> +
> +Mailbox Node:
> + slimpro-mbox: slimpro-mbox@10540000 {
> + compatible = "apm,xgene-slimpro-mbox";
> + reg = <0x0 0x10540000 0x0 0xa000>;
> + #mbox-cells = <1>;
> + interrupts = <0x0 0x0 0x4>,
> + <0x0 0x1 0x4>,
> + <0x0 0x2 0x4>,
> + <0x0 0x3 0x4>,
> + <0x0 0x4 0x4>,
> + <0x0 0x5 0x4>,
> + <0x0 0x6 0x4>,
> + };
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2015-11-10 18:42:53

by Duc Dang

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

On Tue, Nov 10, 2015 at 6:15 AM, Rob Herring <[email protected]> wrote:
> On Mon, Nov 09, 2015 at 10:05:44AM -0800, Duc Dang wrote:
>> This adds the APM X-Gene SLIMpro mailbox device tree
>> node documentation.
>>
>> [dhdang: rebase over 4.3-rc5]
>> Signed-off-by: Feng Kan <[email protected]>
>> Signed-off-by: Duc Dang <[email protected]>
>> ---
>> .../bindings/mailbox/xgene-slimpro-mailbox.txt | 34 ++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
>> new file mode 100644
>> index 0000000..d02a3d8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
>> @@ -0,0 +1,34 @@
>> +The APM X-Gene SLIMpro mailbox is used to communicate messages between
>> +the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
>> +interrupt based door bell mechanism and can exchange simple messages using the
>> +internal registers.
>> +
>> +There are total of 7 interrupts in this mailbox. Each used for an individual
>> +door bell (or mailbox channel).
>> +
>> +Required properties:
>> +- compatible: Should be as "apm, xgene-slimpro-mbox".
> ^
> remove the space

I will fix in next version of the patch.
>
>> +
>> +- reg: Contain the mailbox register address range.
>
> s/Contain/Contains/

Will change.
>
>> +
>> +- interrupts: 7 interrupts must start from 0 to 6, interrupt 0 define the
>> + the interrupt for mailbox channel 0 and interrupt 1 for
>> + mailbox channel 1 and so likewise for the reminder.
>
> s/start/be/

Will change.

>
>> +
>> +- #mbox-cells: only one to specify the mailbox channel number.
>> +
>> +Example:
>> +
>> +Mailbox Node:
>> + slimpro-mbox: slimpro-mbox@10540000 {
>> + compatible = "apm,xgene-slimpro-mbox";
>> + reg = <0x0 0x10540000 0x0 0xa000>;
>> + #mbox-cells = <1>;
>> + interrupts = <0x0 0x0 0x4>,
>> + <0x0 0x1 0x4>,
>> + <0x0 0x2 0x4>,
>> + <0x0 0x3 0x4>,
>> + <0x0 0x4 0x4>,
>> + <0x0 0x5 0x4>,
>> + <0x0 0x6 0x4>,
>> + };
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html


Regards,
Duc Dang.

2015-11-16 19:22:35

by Duc Dang

[permalink] [raw]
Subject: Re: [PATCH v3 0/3] mailbox: Add APM X-Gene platform mailbox driver

On Mon, Nov 9, 2015 at 10:05 AM, Duc Dang <[email protected]> wrote:
> APM X-Gene SoC has a mailbox controller that provides
> communication mechanism for X-Gene Arm64 cores to communicate
> with X-Gene SoC's Cortex M3 (SLIMpro) processor.
>
> X-Gene mailbox controller provides 8 mailbox channels, with
> each channel has a dedicated interrupt line.
>
> Changes since v2:
> - Rebase Feng's patch set over v4.3-rc5
> - Remove uneccessary 'inline' in function definition
> - Use module_platform_driver instead of subsys_initcall
> - Minor coding stype clean up
>
> Changes since v1:
> - Add ACPI support
> - Use defines for reg offset
>
> .../bindings/mailbox/xgene-slimpro-mailbox.txt | 34 +++
> arch/arm64/boot/dts/apm/apm-storm.dtsi | 14 ++
> drivers/mailbox/Kconfig | 9 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/mailbox-xgene-slimpro.c | 264 +++++++++++++++++++++
> 5 files changed, 323 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c
>

Hi Jassi, Paul,

Do you have any comment on this patch set?

> --
> 1.9.1
>
Regards,
Duc dang.

2015-11-20 10:40:05

by Paul Bolle

[permalink] [raw]
Subject: Re: [PATCH v3 0/3] mailbox: Add APM X-Gene platform mailbox driver

Hi Duc,

On ma, 2015-11-16 at 11:22 -0800, Duc Dang wrote:
> Hi Jassi, Paul,
>
> Do you have any comment on this patch set?

Are you perhaps confusing me with another Paul?

(I think I made a minor comment or two on this series about half a year
ago. But I'm not the maintainer here and it's the maintainers verdict
that you're probably interested in mostly. So it's not clear to me why
you specifically ask for my comment.)

Thanks,


Paul Bolle

2015-11-20 11:47:19

by Jassi Brar

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] mailbox: Add support for APM X-Gene platform mailbox driver

On Mon, Nov 9, 2015 at 11:35 PM, Duc Dang <[email protected]> wrote:
> X-Gene mailbox controller provides 8 mailbox channels, with
> each channel has a dedicated interrupt line.
>
> [dhdang: rebase over 4.3-rc5, some minor changes to
> address comment in v2 patch set]
>
Do you want this to go into git logs?

> Signed-off-by: Feng Kan <[email protected]>
> Signed-off-by: Duc Dang <[email protected]>
> ---
here is the right place for any history. And it is good practice to
specify whatever changes were made from last revision.


> +
> +struct slimpro_mbox_chan {
> + struct device *dev;
> + struct mbox_chan *chan;
> + void __iomem *reg;
> + int id;
>
You don't seem to really need 'id'.


> +
> +static void mb_chan_enable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
> +{
> + u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
> +
> + val &= ~mask;
> + writel(val, mb_chan->reg + REG_DB_STATMASK);
> +}
> +
> +static void mb_chan_disable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
> +{
> + u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
> +
> + val |= mask;
> + writel(val, mb_chan->reg + REG_DB_STATMASK);
> +}
> +
These 2 functions are called just once. And do what other drivers
usually inline. Wouldn't it be better to directly set & clear the bit
when needed?

> +static int slimpro_mbox_send_data(struct mbox_chan *chan, void *msg)
> +{
> + struct slimpro_mbox_chan *mb_chan =
> + (struct slimpro_mbox_chan *)chan->con_priv;
> +
You don't need to typecast a void pointer. Here and elsewhere.


> +
> + /* Setup mailbox links */
> + for (i = 0; i < MBOX_CNT; i++) {
> + ctx->mc[i].irq = platform_get_irq(pdev, i);
>
You expect every platform to provide exactly 'MBOX_CNT' irqs and they
must be different (because you don't 'SHARE' when you request).
Usually developers relax either of the conditions.


> +static struct platform_driver slimpro_mbox_driver = {
> + .probe = slimpro_mbox_probe,
> + .remove = slimpro_mbox_remove,
> + .driver = {
> + .name = "xgene-slimpro-mbox",
> + .owner = THIS_MODULE,
>
No need to set .owner.

Cheers.

2015-12-07 09:29:56

by Duc Dang

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] mailbox: Add support for APM X-Gene platform mailbox driver

On Fri, Nov 20, 2015 at 3:47 AM, Jassi Brar <[email protected]> wrote:
> On Mon, Nov 9, 2015 at 11:35 PM, Duc Dang <[email protected]> wrote:
>> X-Gene mailbox controller provides 8 mailbox channels, with
>> each channel has a dedicated interrupt line.
>>
>> [dhdang: rebase over 4.3-rc5, some minor changes to
>> address comment in v2 patch set]
>>
> Do you want this to go into git logs?

Yes.

I will also rebase the patch set to v4.4-rc1.

>
>> Signed-off-by: Feng Kan <[email protected]>
>> Signed-off-by: Duc Dang <[email protected]>
>> ---
> here is the right place for any history. And it is good practice to
> specify whatever changes were made from last revision.

I will add change log into this section in next version of the patch.

>
>
>> +
>> +struct slimpro_mbox_chan {
>> + struct device *dev;
>> + struct mbox_chan *chan;
>> + void __iomem *reg;
>> + int id;
>>
> You don't seem to really need 'id'.

I will remove 'id'.

>
>
>> +
>> +static void mb_chan_enable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
>> +{
>> + u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
>> +
>> + val &= ~mask;
>> + writel(val, mb_chan->reg + REG_DB_STATMASK);
>> +}
>> +
>> +static void mb_chan_disable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
>> +{
>> + u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
>> +
>> + val |= mask;
>> + writel(val, mb_chan->reg + REG_DB_STATMASK);
>> +}
>> +
> These 2 functions are called just once. And do what other drivers
> usually inline. Wouldn't it be better to directly set & clear the bit
> when needed?

I will fold these 2 functions into their callers and then get rid of them.

>
>> +static int slimpro_mbox_send_data(struct mbox_chan *chan, void *msg)
>> +{
>> + struct slimpro_mbox_chan *mb_chan =
>> + (struct slimpro_mbox_chan *)chan->con_priv;
>> +
> You don't need to typecast a void pointer. Here and elsewhere.

I will remove all the typecast of a void pointer in the driver.

>
>
>> +
>> + /* Setup mailbox links */
>> + for (i = 0; i < MBOX_CNT; i++) {
>> + ctx->mc[i].irq = platform_get_irq(pdev, i);
>>
> You expect every platform to provide exactly 'MBOX_CNT' irqs and they
> must be different (because you don't 'SHARE' when you request).
> Usually developers relax either of the conditions.

I will relax the MBOX_CNT irqs restriction: Platform can provide less
than MBOX_CNT irqs, which also means less than MBOX_CNT mailbox
channels.

>
>
>> +static struct platform_driver slimpro_mbox_driver = {
>> + .probe = slimpro_mbox_probe,
>> + .remove = slimpro_mbox_remove,
>> + .driver = {
>> + .name = "xgene-slimpro-mbox",
>> + .owner = THIS_MODULE,
>>
> No need to set .owner.

I will remove it.

>
> Cheers.

Regards,
Duc Dang.