[PATCH v2 1/2] remoteproc: microblaze: Document device tree bindings

[PATCH v2 1/2] remoteproc: microblaze: Document device tree bindings

[Michal Simek]

Add device tree binding documentation for the Microblaze remoteproc
on Xilinx Zynq.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Applied changed reported by Mark Rutland here
  https://lkml.org/lkml/2015/1/19/198

 .../bindings/remoteproc/mb_remoteproc.txt          | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt

diff --git a/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt b/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
new file mode 100644
index 000000000000..3a454f01f5f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/mb_remoteproc.txt
@@ -0,0 +1,45 @@
+Xilinx ARM-Microblaze remoteproc driver
+
+A Microblaze is added to the programmable logic on Xilinx Zynq.
+The Microblaze is connected with PS block via axi bus connected to PS HP port
+to ensure access to PS DDR.
+Communication channels are done via soft GPIO IP connected to PS block
+and to Microblaze. There are also 2 gpio control signals reset and debug
+which are used for resetting Microblaze.
+
+Required properties:
+- compatible : Should be "xlnx,mb-remoteproc"
+- reg : Address and length of the ddr address space
+- bram: Phandle to bram controller which can access Microblaze BRAM
+- bram-firmware : Microblaze BRAM bootloader name
+- firmware : Default firmware name which can be override by
+	     "firmware" module parameter
+- reset-gpio : Gpio phandle which reset Microblaze remoteproc
+- debug-gpio : Gpio phandle which setup Microblaze to debug state
+- ipino-gpio : Gpio phandle for Microblaze to ARM communication
+- vring0-gpio : Gpio phandle for ARM to Microblaze communication vring 0
+- vring1-gpio : Gpio phandle for ARM to Microblaze communication vring 1
+
+Microblaze SoC can be also connected to the PS block via an axi bus.
+That's why there is the option to allocate interrupts for Microblaze use only.
+The driver will allocate interrupts to itself and Microblaze sw has to ensure
+that interrupts are properly enabled and handled by Microblaze interrupt
+controller.
+
+Optional properties:
+ - interrupts : Interrupt mapping for remoteproc
+ - interrupt-parent : Phandle for the interrupt controller
+
+Example:
+mb_remoteproc@800000 {
+	compatible = "xlnx,mb-remoteproc";
+	reg = < 0x8000000 0x8000000 >;
+	bram = <&axi_bram_ctrl_0>;
+	bram-firmware = "mb.bin";
+	firmware = "image.elf";
+	reset-gpio = <&zynq_gpio_reset 1 0>;
+	debug-gpio = <&zynq_gpio_reset 0 0>;
+	ipino-gpio = <&zynq_gpio_vring 0 0>;
+	vring0-gpio = <&zynq_gpio_vring 1 0>;
+	vring1-gpio = <&zynq_gpio_vring 2 0>;
+};
-- 
1.8.2.3

[PATCH v2 2/2] remoteproc: microblaze: Add support for microblaze on Zynq

[Michal Simek]

Xilinx Microblaze is placed in programmable logic on Xilinx
Zynq architecture. Driver requires specific HW setting
described in DT binding.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Rebase on 4.0-rc1
- remove DEBUG_SG dependency - Reported-by: Nathan Lynch
  <Nathan_Lynch@mentor.com>
- Changes caused by DT binding change Reported-by: Mark Rutland
  <mark.rutland@arm.com> - https://lkml.org/lkml/2015/1/19/198
- Use const in mb_remoteproc_match[] reported by checkpatch.pl.

 MAINTAINERS                        |   1 +
 drivers/remoteproc/Kconfig         |  11 +
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/mb_remoteproc.c | 427 +++++++++++++++++++++++++++++++++++++
 4 files changed, 440 insertions(+)
 create mode 100644 drivers/remoteproc/mb_remoteproc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ddc5a8cf9a8a..451a1f0cf4a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1606,6 +1606,7 @@ F:	drivers/clocksource/cadence_ttc_timer.c
 F:	drivers/i2c/busses/i2c-cadence.c
 F:	drivers/mmc/host/sdhci-of-arasan.c
 F:	drivers/edac/synopsys_edac.c
+F:	drivers/remoteproc/mb_remoteproc.c
 
 ARM SMMU DRIVER
 M:	Will Deacon <will.deacon@arm.com>
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 5e343bab9458..d119c2728a7b 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
 	  It's safe to say n here if you're not interested in multimedia
 	  offloading.
 
+config MB_REMOTEPROC
+	tristate "Support Microblaze remoteproc"
+	depends on ARCH_ZYNQ
+	select GPIO_XILINX
+	select REMOTEPROC
+	select RPMSG
+	default m
+	help
+	  Say y here to support Xilinx Microblaze remote processors
+	  on the Xilinx Zynq.
+
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ac2ff75686d2..40e247ffac34 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,3 +10,4 @@ remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
+obj-$(CONFIG_MB_REMOTEPROC)		+= mb_remoteproc.o
diff --git a/drivers/remoteproc/mb_remoteproc.c b/drivers/remoteproc/mb_remoteproc.c
new file mode 100644
index 000000000000..7febf32abafc
--- /dev/null
+++ b/drivers/remoteproc/mb_remoteproc.c
@@ -0,0 +1,427 @@
+/*
+ * Microblaze Remote Processor driver
+ *
+ * Copyright (C) 2012 - 2015 Michal Simek <monstr@monstr.eu>
+ * Copyright (C) 2013 - 2015 Xilinx, Inc.
+ * Copyright (C) 2012 PetaLogix
+ *
+ * Based on origin OMAP Remote Processor driver
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc.
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/remoteproc.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/smp.h>
+#include <linux/irqchip/arm-gic.h>
+#include <asm/outercache.h>
+#include <asm/cacheflush.h>
+#include <linux/slab.h>
+#include <linux/cpu.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+
+#include "remoteproc_internal.h"
+
+/* Module parameter */
+static char *firmware;
+
+/* Private data */
+struct mb_rproc_pdata {
+	struct rproc *rproc;
+	u32 mem_start;
+	u32 mem_end;
+	int reset_gpio;
+	int mb_debug_gpio;
+	int ipi;
+	int vring0;
+	int vring1;
+	void __iomem *vbase;
+	const unsigned char *bootloader;
+};
+
+/* Store rproc for IPI handler */
+static struct platform_device *remoteprocdev;
+static struct work_struct workqueue;
+
+static void handle_event(struct work_struct *work)
+{
+	struct mb_rproc_pdata *local = platform_get_drvdata(remoteprocdev);
+
+	flush_cache_all();
+	outer_flush_range(local->mem_start, local->mem_end);
+
+	if (rproc_vq_interrupt(local->rproc, 0) == IRQ_NONE)
+		dev_info(&remoteprocdev->dev, "no message found in vqid 0\n");
+}
+
+static irqreturn_t ipi_kick(int irq, void *dev_id)
+{
+	dev_dbg(&remoteprocdev->dev, "KICK Linux because of pending message\n");
+	schedule_work(&workqueue);
+	dev_dbg(&remoteprocdev->dev, "KICK Linux handled\n");
+
+	return IRQ_HANDLED;
+}
+
+static int mb_rproc_start(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+	const struct firmware *fw;
+	int ret;
+
+	dev_info(dev, "%s\n", __func__);
+	INIT_WORK(&workqueue, handle_event);
+
+	flush_cache_all();
+	outer_flush_range(local->mem_start, local->mem_end);
+
+	remoteprocdev = pdev;
+
+	ret = request_firmware(&fw, local->bootloader, &pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "request_firmware failed\n");
+		return ret;
+	}
+	/* Copy bootloader to memory */
+	memcpy(local->vbase, fw->data, fw->size);
+	release_firmware(fw);
+
+	/* Just for sure synchronize memories */
+	dsb();
+
+	/* Release Microblaze from reset */
+	gpio_set_value(local->reset_gpio, 0);
+
+	return 0;
+}
+
+/* kick a firmware */
+static void mb_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+
+	dev_dbg(dev, "KICK Firmware to start send messages vqid %d\n", vqid);
+
+	flush_cache_all();
+	outer_flush_all();
+
+	/* Send swirq to firmware */
+	gpio_set_value(local->vring0, 0);
+	gpio_set_value(local->vring1, 0);
+	dsb();
+
+	if (!vqid) {
+		udelay(500);
+		gpio_set_value(local->vring0, 1);
+		dsb();
+	} else {
+		udelay(100);
+		gpio_set_value(local->vring1, 1);
+		dsb();
+	}
+}
+
+/* power off the remote processor */
+static int mb_rproc_stop(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+
+	/* Setup MB to the state where all memory transactions are done */
+	gpio_set_value(local->mb_debug_gpio, 1);
+	dsb(); /* Be sure that this write has been done */
+	/*
+	 * This should be enough to ensure one CLK as
+	 * it is written in MB ref guide
+	 */
+	gpio_set_value(local->mb_debug_gpio, 0);
+
+	udelay(1000); /* Wait some time to finish all mem transactions */
+
+	/* Add Microblaze to reset state */
+	gpio_set_value(local->reset_gpio, 1);
+
+	/* No reason to wait that operations where done */
+	return 0;
+}
+
+static struct rproc_ops mb_rproc_ops = {
+	.start		= mb_rproc_start,
+	.stop		= mb_rproc_stop,
+	.kick		= mb_rproc_kick,
+};
+
+/* Just to detect bug if interrupt forwarding is broken */
+static irqreturn_t mb_remoteproc_interrupt(int irq, void *dev_id)
+{
+	struct device *dev = dev_id;
+
+	dev_err(dev, "GIC IRQ %d is not forwarded correctly\n", irq);
+
+	return IRQ_HANDLED;
+}
+
+static int mb_remoteproc_probe(struct platform_device *pdev)
+{
+	const unsigned char *prop;
+	struct platform_device *bram_pdev;
+	struct device_node *bram_dev;
+	struct resource *res; /* IO mem resources */
+	int ret = 0;
+	int count = 0;
+	struct mb_rproc_pdata *local;
+
+	local = devm_kzalloc(&pdev->dev, sizeof(*local), GFP_KERNEL);
+	if (!local)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, local);
+
+	/* Declare memory for firmware */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "invalid address\n");
+		return -ENODEV;
+	}
+
+	local->mem_start = res->start;
+	local->mem_end = res->end;
+
+	/* Alloc phys addr from 0 to max_addr for firmware */
+	ret = dma_declare_coherent_memory(&pdev->dev, local->mem_start,
+		local->mem_start, local->mem_end - local->mem_start + 1,
+		DMA_MEMORY_IO);
+	if (!ret) {
+		dev_err(&pdev->dev, "dma_declare_coherent_memory failed\n");
+		return -ENOMEM;
+	}
+
+	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(&pdev->dev, "dma_set_coherent_mask: %d\n", ret);
+		goto dma_mask_fault;
+	}
+
+	/* Alloc IRQ based on DTS to be sure that no other driver will use it */
+	while (1) {
+		int irq;
+		/* Allocating shared IRQs will ensure that any module will
+		 * use these IRQs */
+		irq = platform_get_irq(pdev, count++);
+		if (irq == -ENXIO || irq == -EINVAL)
+			break;
+		ret = devm_request_irq(&pdev->dev, irq, mb_remoteproc_interrupt,
+				       0, dev_name(&pdev->dev), &pdev->dev);
+		if (ret) {
+			dev_err(&pdev->dev, "IRQ %d already allocated\n", irq);
+			goto dma_mask_fault;
+		}
+
+		dev_info(&pdev->dev, "%d: Alloc irq: %d\n", count, irq);
+	}
+
+	/* Find out reset gpio and keep microblaze in reset */
+	local->reset_gpio = of_get_named_gpio(pdev->dev.of_node,
+					      "reset-gpio", 0);
+	if (local->reset_gpio < 0) {
+		dev_err(&pdev->dev, "reset-gpio property not found\n");
+		ret = local->reset_gpio;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->reset_gpio,
+				    GPIOF_OUT_INIT_HIGH, "mb_reset");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+
+	/* Find out reset gpio and keep microblaze in reset */
+	local->mb_debug_gpio = of_get_named_gpio(pdev->dev.of_node,
+						 "debug-gpio", 0);
+	if (local->mb_debug_gpio < 0) {
+		dev_err(&pdev->dev, "mb-debug-gpio property not found\n");
+		ret = local->mb_debug_gpio;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->mb_debug_gpio,
+				    GPIOF_OUT_INIT_LOW, "mb_debug");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio debug pin\n");
+		goto dma_mask_fault;
+	}
+
+	/* IPI number for getting irq from firmware */
+	local->ipi = of_get_named_gpio(pdev->dev.of_node, "ipino-gpio", 0);
+	if (local->ipi < 0) {
+		dev_err(&pdev->dev, "ipi-gpio property not found\n");
+		ret = local->ipi;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->ipi, GPIOF_IN, "mb_ipi");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+	ret = devm_request_irq(&pdev->dev, gpio_to_irq(local->ipi),
+			       ipi_kick, IRQF_SHARED|IRQF_TRIGGER_RISING,
+			       dev_name(&pdev->dev), local);
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ %d already allocated\n", local->ipi);
+		goto dma_mask_fault;
+	}
+
+	/* Find out vring0 pin */
+	local->vring0 = of_get_named_gpio(pdev->dev.of_node, "vring0-gpio", 0);
+	if (local->vring0 < 0) {
+		dev_err(&pdev->dev, "reset-gpio property not found\n");
+		ret = local->vring0;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->vring0,
+				    GPIOF_DIR_OUT, "mb_vring0");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+
+	/* Find out vring1 pin */
+	local->vring1 = of_get_named_gpio(pdev->dev.of_node, "vring1-gpio", 0);
+	if (local->vring1 < 0) {
+		dev_err(&pdev->dev, "reset-gpio property not found\n");
+		ret = local->vring1;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->vring1,
+				    GPIOF_DIR_OUT, "mb_vring1");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+
+	/* Allocate bram device */
+	bram_dev = of_parse_phandle(pdev->dev.of_node, "bram", 0);
+	if (!bram_dev) {
+		dev_err(&pdev->dev, "Please specify bram connection\n");
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+	bram_pdev = of_find_device_by_node(bram_dev);
+	if (!bram_pdev) {
+		dev_err(&pdev->dev, "BRAM device hasn't found\n");
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+	res = platform_get_resource(bram_pdev, IORESOURCE_MEM, 0);
+	local->vbase = devm_ioremap_resource(&pdev->dev, res);
+	if (!local->vbase) {
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+
+	/* Load simple bootloader to bram */
+	local->bootloader = of_get_property(pdev->dev.of_node,
+					    "bram-firmware", NULL);
+	if (!local->bootloader) {
+		dev_err(&pdev->dev, "Please specify BRAM firmware\n");
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+
+	dev_info(&pdev->dev, "Using microblaze BRAM bootloader: %s\n",
+		 local->bootloader);
+
+	/* Module param firmware first */
+	if (firmware)
+		prop = firmware;
+	else
+		prop = of_get_property(pdev->dev.of_node, "firmware", NULL);
+
+	if (prop) {
+		dev_info(&pdev->dev, "Using firmware: %s\n", prop);
+		local->rproc = rproc_alloc(&pdev->dev, dev_name(&pdev->dev),
+				&mb_rproc_ops, prop, sizeof(struct rproc));
+		if (!local->rproc) {
+			dev_err(&pdev->dev, "rproc allocation failed\n");
+			ret = -ENODEV;
+			goto dma_mask_fault;
+		}
+
+		ret = rproc_add(local->rproc);
+		if (ret) {
+			dev_err(&pdev->dev, "rproc registration failed\n");
+			rproc_put(local->rproc);
+			goto dma_mask_fault;
+		}
+		return 0;
+	}
+
+	ret = -ENODEV;
+
+dma_mask_fault:
+	dma_release_declared_memory(&pdev->dev);
+
+	return ret;
+}
+
+static int mb_remoteproc_remove(struct platform_device *pdev)
+{
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+
+	dev_info(&pdev->dev, "%s\n", __func__);
+
+	dma_release_declared_memory(&pdev->dev);
+
+	rproc_del(local->rproc);
+	rproc_put(local->rproc);
+
+	return 0;
+}
+
+/* Match table for OF platform binding */
+static const struct of_device_id mb_remoteproc_match[] = {
+	{ .compatible = "xlnx,mb-remoteproc", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, mb_remoteproc_match);
+
+static struct platform_driver mb_remoteproc_driver = {
+	.probe = mb_remoteproc_probe,
+	.remove = mb_remoteproc_remove,
+	.driver = {
+		.name = "mb-remoteproc",
+		.of_match_table = mb_remoteproc_match,
+	},
+};
+module_platform_driver(mb_remoteproc_driver);
+
+module_param(firmware, charp, 0);
+MODULE_PARM_DESC(firmware, "Override the firmware image name. Default value in DTS.");
+
+MODULE_AUTHOR("Michal Simek <monstr@monstr.eu");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Microblaze remote processor control driver");
-- 
1.8.2.3

[PATCH v2 2/2] remoteproc: microblaze: Add support for microblaze on Zynq

[Michal Simek]

Xilinx Microblaze is placed in programmable logic on Xilinx
Zynq architecture. Driver requires specific HW setting
described in DT binding.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Rebase on 4.0-rc1
- remove DEBUG_SG dependency - Reported-by: Nathan Lynch
  <Nathan_Lynch@mentor.com>
- Changes caused by DT binding change Reported-by: Mark Rutland
  <mark.rutland@arm.com> - https://lkml.org/lkml/2015/1/19/198
- Use const in mb_remoteproc_match[] reported by checkpatch.pl.

 MAINTAINERS                        |   1 +
 drivers/remoteproc/Kconfig         |  11 +
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/mb_remoteproc.c | 427 +++++++++++++++++++++++++++++++++++++
 4 files changed, 440 insertions(+)
 create mode 100644 drivers/remoteproc/mb_remoteproc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ddc5a8cf9a8a..451a1f0cf4a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1606,6 +1606,7 @@ F:	drivers/clocksource/cadence_ttc_timer.c
 F:	drivers/i2c/busses/i2c-cadence.c
 F:	drivers/mmc/host/sdhci-of-arasan.c
 F:	drivers/edac/synopsys_edac.c
+F:	drivers/remoteproc/mb_remoteproc.c
 
 ARM SMMU DRIVER
 M:	Will Deacon <will.deacon@arm.com>
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 5e343bab9458..d119c2728a7b 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
 	  It's safe to say n here if you're not interested in multimedia
 	  offloading.
 
+config MB_REMOTEPROC
+	tristate "Support Microblaze remoteproc"
+	depends on ARCH_ZYNQ
+	select GPIO_XILINX
+	select REMOTEPROC
+	select RPMSG
+	default m
+	help
+	  Say y here to support Xilinx Microblaze remote processors
+	  on the Xilinx Zynq.
+
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ac2ff75686d2..40e247ffac34 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,3 +10,4 @@ remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
+obj-$(CONFIG_MB_REMOTEPROC)		+= mb_remoteproc.o
diff --git a/drivers/remoteproc/mb_remoteproc.c b/drivers/remoteproc/mb_remoteproc.c
new file mode 100644
index 000000000000..7febf32abafc
--- /dev/null
+++ b/drivers/remoteproc/mb_remoteproc.c
@@ -0,0 +1,427 @@
+/*
+ * Microblaze Remote Processor driver
+ *
+ * Copyright (C) 2012 - 2015 Michal Simek <monstr@monstr.eu>
+ * Copyright (C) 2013 - 2015 Xilinx, Inc.
+ * Copyright (C) 2012 PetaLogix
+ *
+ * Based on origin OMAP Remote Processor driver
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc.
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/remoteproc.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/smp.h>
+#include <linux/irqchip/arm-gic.h>
+#include <asm/outercache.h>
+#include <asm/cacheflush.h>
+#include <linux/slab.h>
+#include <linux/cpu.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+
+#include "remoteproc_internal.h"
+
+/* Module parameter */
+static char *firmware;
+
+/* Private data */
+struct mb_rproc_pdata {
+	struct rproc *rproc;
+	u32 mem_start;
+	u32 mem_end;
+	int reset_gpio;
+	int mb_debug_gpio;
+	int ipi;
+	int vring0;
+	int vring1;
+	void __iomem *vbase;
+	const unsigned char *bootloader;
+};
+
+/* Store rproc for IPI handler */
+static struct platform_device *remoteprocdev;
+static struct work_struct workqueue;
+
+static void handle_event(struct work_struct *work)
+{
+	struct mb_rproc_pdata *local = platform_get_drvdata(remoteprocdev);
+
+	flush_cache_all();
+	outer_flush_range(local->mem_start, local->mem_end);
+
+	if (rproc_vq_interrupt(local->rproc, 0) == IRQ_NONE)
+		dev_info(&remoteprocdev->dev, "no message found in vqid 0\n");
+}
+
+static irqreturn_t ipi_kick(int irq, void *dev_id)
+{
+	dev_dbg(&remoteprocdev->dev, "KICK Linux because of pending message\n");
+	schedule_work(&workqueue);
+	dev_dbg(&remoteprocdev->dev, "KICK Linux handled\n");
+
+	return IRQ_HANDLED;
+}
+
+static int mb_rproc_start(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+	const struct firmware *fw;
+	int ret;
+
+	dev_info(dev, "%s\n", __func__);
+	INIT_WORK(&workqueue, handle_event);
+
+	flush_cache_all();
+	outer_flush_range(local->mem_start, local->mem_end);
+
+	remoteprocdev = pdev;
+
+	ret = request_firmware(&fw, local->bootloader, &pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "request_firmware failed\n");
+		return ret;
+	}
+	/* Copy bootloader to memory */
+	memcpy(local->vbase, fw->data, fw->size);
+	release_firmware(fw);
+
+	/* Just for sure synchronize memories */
+	dsb();
+
+	/* Release Microblaze from reset */
+	gpio_set_value(local->reset_gpio, 0);
+
+	return 0;
+}
+
+/* kick a firmware */
+static void mb_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+
+	dev_dbg(dev, "KICK Firmware to start send messages vqid %d\n", vqid);
+
+	flush_cache_all();
+	outer_flush_all();
+
+	/* Send swirq to firmware */
+	gpio_set_value(local->vring0, 0);
+	gpio_set_value(local->vring1, 0);
+	dsb();
+
+	if (!vqid) {
+		udelay(500);
+		gpio_set_value(local->vring0, 1);
+		dsb();
+	} else {
+		udelay(100);
+		gpio_set_value(local->vring1, 1);
+		dsb();
+	}
+}
+
+/* power off the remote processor */
+static int mb_rproc_stop(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+
+	/* Setup MB to the state where all memory transactions are done */
+	gpio_set_value(local->mb_debug_gpio, 1);
+	dsb(); /* Be sure that this write has been done */
+	/*
+	 * This should be enough to ensure one CLK as
+	 * it is written in MB ref guide
+	 */
+	gpio_set_value(local->mb_debug_gpio, 0);
+
+	udelay(1000); /* Wait some time to finish all mem transactions */
+
+	/* Add Microblaze to reset state */
+	gpio_set_value(local->reset_gpio, 1);
+
+	/* No reason to wait that operations where done */
+	return 0;
+}
+
+static struct rproc_ops mb_rproc_ops = {
+	.start		= mb_rproc_start,
+	.stop		= mb_rproc_stop,
+	.kick		= mb_rproc_kick,
+};
+
+/* Just to detect bug if interrupt forwarding is broken */
+static irqreturn_t mb_remoteproc_interrupt(int irq, void *dev_id)
+{
+	struct device *dev = dev_id;
+
+	dev_err(dev, "GIC IRQ %d is not forwarded correctly\n", irq);
+
+	return IRQ_HANDLED;
+}
+
+static int mb_remoteproc_probe(struct platform_device *pdev)
+{
+	const unsigned char *prop;
+	struct platform_device *bram_pdev;
+	struct device_node *bram_dev;
+	struct resource *res; /* IO mem resources */
+	int ret = 0;
+	int count = 0;
+	struct mb_rproc_pdata *local;
+
+	local = devm_kzalloc(&pdev->dev, sizeof(*local), GFP_KERNEL);
+	if (!local)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, local);
+
+	/* Declare memory for firmware */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "invalid address\n");
+		return -ENODEV;
+	}
+
+	local->mem_start = res->start;
+	local->mem_end = res->end;
+
+	/* Alloc phys addr from 0 to max_addr for firmware */
+	ret = dma_declare_coherent_memory(&pdev->dev, local->mem_start,
+		local->mem_start, local->mem_end - local->mem_start + 1,
+		DMA_MEMORY_IO);
+	if (!ret) {
+		dev_err(&pdev->dev, "dma_declare_coherent_memory failed\n");
+		return -ENOMEM;
+	}
+
+	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(&pdev->dev, "dma_set_coherent_mask: %d\n", ret);
+		goto dma_mask_fault;
+	}
+
+	/* Alloc IRQ based on DTS to be sure that no other driver will use it */
+	while (1) {
+		int irq;
+		/* Allocating shared IRQs will ensure that any module will
+		 * use these IRQs */
+		irq = platform_get_irq(pdev, count++);
+		if (irq == -ENXIO || irq == -EINVAL)
+			break;
+		ret = devm_request_irq(&pdev->dev, irq, mb_remoteproc_interrupt,
+				       0, dev_name(&pdev->dev), &pdev->dev);
+		if (ret) {
+			dev_err(&pdev->dev, "IRQ %d already allocated\n", irq);
+			goto dma_mask_fault;
+		}
+
+		dev_info(&pdev->dev, "%d: Alloc irq: %d\n", count, irq);
+	}
+
+	/* Find out reset gpio and keep microblaze in reset */
+	local->reset_gpio = of_get_named_gpio(pdev->dev.of_node,
+					      "reset-gpio", 0);
+	if (local->reset_gpio < 0) {
+		dev_err(&pdev->dev, "reset-gpio property not found\n");
+		ret = local->reset_gpio;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->reset_gpio,
+				    GPIOF_OUT_INIT_HIGH, "mb_reset");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+
+	/* Find out reset gpio and keep microblaze in reset */
+	local->mb_debug_gpio = of_get_named_gpio(pdev->dev.of_node,
+						 "debug-gpio", 0);
+	if (local->mb_debug_gpio < 0) {
+		dev_err(&pdev->dev, "mb-debug-gpio property not found\n");
+		ret = local->mb_debug_gpio;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->mb_debug_gpio,
+				    GPIOF_OUT_INIT_LOW, "mb_debug");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio debug pin\n");
+		goto dma_mask_fault;
+	}
+
+	/* IPI number for getting irq from firmware */
+	local->ipi = of_get_named_gpio(pdev->dev.of_node, "ipino-gpio", 0);
+	if (local->ipi < 0) {
+		dev_err(&pdev->dev, "ipi-gpio property not found\n");
+		ret = local->ipi;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->ipi, GPIOF_IN, "mb_ipi");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+	ret = devm_request_irq(&pdev->dev, gpio_to_irq(local->ipi),
+			       ipi_kick, IRQF_SHARED|IRQF_TRIGGER_RISING,
+			       dev_name(&pdev->dev), local);
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ %d already allocated\n", local->ipi);
+		goto dma_mask_fault;
+	}
+
+	/* Find out vring0 pin */
+	local->vring0 = of_get_named_gpio(pdev->dev.of_node, "vring0-gpio", 0);
+	if (local->vring0 < 0) {
+		dev_err(&pdev->dev, "reset-gpio property not found\n");
+		ret = local->vring0;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->vring0,
+				    GPIOF_DIR_OUT, "mb_vring0");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+
+	/* Find out vring1 pin */
+	local->vring1 = of_get_named_gpio(pdev->dev.of_node, "vring1-gpio", 0);
+	if (local->vring1 < 0) {
+		dev_err(&pdev->dev, "reset-gpio property not found\n");
+		ret = local->vring1;
+		goto dma_mask_fault;
+	}
+	ret = devm_gpio_request_one(&pdev->dev, local->vring1,
+				    GPIOF_DIR_OUT, "mb_vring1");
+	if (ret) {
+		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
+		goto dma_mask_fault;
+	}
+
+	/* Allocate bram device */
+	bram_dev = of_parse_phandle(pdev->dev.of_node, "bram", 0);
+	if (!bram_dev) {
+		dev_err(&pdev->dev, "Please specify bram connection\n");
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+	bram_pdev = of_find_device_by_node(bram_dev);
+	if (!bram_pdev) {
+		dev_err(&pdev->dev, "BRAM device hasn't found\n");
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+	res = platform_get_resource(bram_pdev, IORESOURCE_MEM, 0);
+	local->vbase = devm_ioremap_resource(&pdev->dev, res);
+	if (!local->vbase) {
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+
+	/* Load simple bootloader to bram */
+	local->bootloader = of_get_property(pdev->dev.of_node,
+					    "bram-firmware", NULL);
+	if (!local->bootloader) {
+		dev_err(&pdev->dev, "Please specify BRAM firmware\n");
+		ret = -ENODEV;
+		goto dma_mask_fault;
+	}
+
+	dev_info(&pdev->dev, "Using microblaze BRAM bootloader: %s\n",
+		 local->bootloader);
+
+	/* Module param firmware first */
+	if (firmware)
+		prop = firmware;
+	else
+		prop = of_get_property(pdev->dev.of_node, "firmware", NULL);
+
+	if (prop) {
+		dev_info(&pdev->dev, "Using firmware: %s\n", prop);
+		local->rproc = rproc_alloc(&pdev->dev, dev_name(&pdev->dev),
+				&mb_rproc_ops, prop, sizeof(struct rproc));
+		if (!local->rproc) {
+			dev_err(&pdev->dev, "rproc allocation failed\n");
+			ret = -ENODEV;
+			goto dma_mask_fault;
+		}
+
+		ret = rproc_add(local->rproc);
+		if (ret) {
+			dev_err(&pdev->dev, "rproc registration failed\n");
+			rproc_put(local->rproc);
+			goto dma_mask_fault;
+		}
+		return 0;
+	}
+
+	ret = -ENODEV;
+
+dma_mask_fault:
+	dma_release_declared_memory(&pdev->dev);
+
+	return ret;
+}
+
+static int mb_remoteproc_remove(struct platform_device *pdev)
+{
+	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
+
+	dev_info(&pdev->dev, "%s\n", __func__);
+
+	dma_release_declared_memory(&pdev->dev);
+
+	rproc_del(local->rproc);
+	rproc_put(local->rproc);
+
+	return 0;
+}
+
+/* Match table for OF platform binding */
+static const struct of_device_id mb_remoteproc_match[] = {
+	{ .compatible = "xlnx,mb-remoteproc", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, mb_remoteproc_match);
+
+static struct platform_driver mb_remoteproc_driver = {
+	.probe = mb_remoteproc_probe,
+	.remove = mb_remoteproc_remove,
+	.driver = {
+		.name = "mb-remoteproc",
+		.of_match_table = mb_remoteproc_match,
+	},
+};
+module_platform_driver(mb_remoteproc_driver);
+
+module_param(firmware, charp, 0);
+MODULE_PARM_DESC(firmware, "Override the firmware image name. Default value in DTS.");
+
+MODULE_AUTHOR("Michal Simek <monstr@monstr.eu");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Microblaze remote processor control driver");
-- 
1.8.2.3

Re: [PATCH v2 2/2] remoteproc: microblaze: Add support for microblaze on Zynq

[Josh Cartwright]

Hey Michal-

I've got a few comments on this.

On Mon, Feb 23, 2015 at 12:37:26PM +0100, Michal Simek wrote:
> Xilinx Microblaze is placed in programmable logic on Xilinx
> Zynq architecture. Driver requires specific HW setting
> described in DT binding.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
[..]
> +++ b/drivers/remoteproc/Kconfig
> @@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
>  	  It's safe to say n here if you're not interested in multimedia
>  	  offloading.
>  
> +config MB_REMOTEPROC
> +	tristate "Support Microblaze remoteproc"
> +	depends on ARCH_ZYNQ
> +	select GPIO_XILINX

Why?  The driver doesn't (and shouldn't) know which GPIO controller is
being used.

> +	select REMOTEPROC
> +	select RPMSG
> +	default m
> +	help
> +	  Say y here to support Xilinx Microblaze remote processors
> +	  on the Xilinx Zynq.
> +
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ac2ff75686d2..40e247ffac34 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -10,3 +10,4 @@ remoteproc-y				+= remoteproc_elf_loader.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
> +obj-$(CONFIG_MB_REMOTEPROC)		+= mb_remoteproc.o
> diff --git a/drivers/remoteproc/mb_remoteproc.c b/drivers/remoteproc/mb_remoteproc.c
> new file mode 100644
> index 000000000000..7febf32abafc
> --- /dev/null
> +++ b/drivers/remoteproc/mb_remoteproc.c
[..]
> +/* Private data */
> +struct mb_rproc_pdata {
> +	struct rproc *rproc;
> +	u32 mem_start;
> +	u32 mem_end;
> +	int reset_gpio;
> +	int mb_debug_gpio;
> +	int ipi;
> +	int vring0;
> +	int vring1;
> +	void __iomem *vbase;
> +	const unsigned char *bootloader;
> +};
> +
> +/* Store rproc for IPI handler */
> +static struct platform_device *remoteprocdev;
> +static struct work_struct workqueue;

Roll the workqueue into mb_rproc_pdata?  I see no reason that you need
this global state.

> +static void handle_event(struct work_struct *work)
> +{
> +	struct mb_rproc_pdata *local = platform_get_drvdata(remoteprocdev);
> +
> +	flush_cache_all();
> +	outer_flush_range(local->mem_start, local->mem_end);
> +
> +	if (rproc_vq_interrupt(local->rproc, 0) == IRQ_NONE)
> +		dev_info(&remoteprocdev->dev, "no message found in vqid 0\n");
> +}
> +
> +static irqreturn_t ipi_kick(int irq, void *dev_id)
> +{
> +	dev_dbg(&remoteprocdev->dev, "KICK Linux because of pending message\n");
> +	schedule_work(&workqueue);

Perhaps it's easier to use request_threaded_irq() as your mechanism of
deferring instead of a workqueue.

> +	dev_dbg(&remoteprocdev->dev, "KICK Linux handled\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mb_rproc_start(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +	const struct firmware *fw;
> +	int ret;
> +
> +	dev_info(dev, "%s\n", __func__);
> +	INIT_WORK(&workqueue, handle_event);
> +
> +	flush_cache_all();
> +	outer_flush_range(local->mem_start, local->mem_end);
> +
> +	remoteprocdev = pdev;
> +
> +	ret = request_firmware(&fw, local->bootloader, &pdev->dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "request_firmware failed\n");
> +		return ret;
> +	}
> +	/* Copy bootloader to memory */
> +	memcpy(local->vbase, fw->data, fw->size);

sparse is not going to like you.  How are you guaranteeing coherence?

> +	release_firmware(fw);
> +
> +	/* Just for sure synchronize memories */
> +	dsb();
> +
> +	/* Release Microblaze from reset */
> +	gpio_set_value(local->reset_gpio, 0);
> +
> +	return 0;
> +}
> +
> +/* kick a firmware */
> +static void mb_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +
> +	dev_dbg(dev, "KICK Firmware to start send messages vqid %d\n", vqid);
> +
> +	flush_cache_all();
> +	outer_flush_all();
> +
> +	/* Send swirq to firmware */
> +	gpio_set_value(local->vring0, 0);
> +	gpio_set_value(local->vring1, 0);
> +	dsb();
> +
> +	if (!vqid) {
> +		udelay(500);
> +		gpio_set_value(local->vring0, 1);
> +		dsb();
> +	} else {
> +		udelay(100);
> +		gpio_set_value(local->vring1, 1);
> +		dsb();
> +	}
> +}
> +
> +/* power off the remote processor */
> +static int mb_rproc_stop(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +
> +	/* Setup MB to the state where all memory transactions are done */
> +	gpio_set_value(local->mb_debug_gpio, 1);
> +	dsb(); /* Be sure that this write has been done */
> +	/*
> +	 * This should be enough to ensure one CLK as
> +	 * it is written in MB ref guide
> +	 */
> +	gpio_set_value(local->mb_debug_gpio, 0);
> +
> +	udelay(1000); /* Wait some time to finish all mem transactions */
> +
> +	/* Add Microblaze to reset state */
> +	gpio_set_value(local->reset_gpio, 1);
> +
> +	/* No reason to wait that operations where done */
> +	return 0;
> +}
> +
> +static struct rproc_ops mb_rproc_ops = {

const?

> +	.start		= mb_rproc_start,
> +	.stop		= mb_rproc_stop,
> +	.kick		= mb_rproc_kick,
> +};
> +
> +/* Just to detect bug if interrupt forwarding is broken */
> +static irqreturn_t mb_remoteproc_interrupt(int irq, void *dev_id)
> +{
> +	struct device *dev = dev_id;
> +
> +	dev_err(dev, "GIC IRQ %d is not forwarded correctly\n", irq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mb_remoteproc_probe(struct platform_device *pdev)
> +{
> +	const unsigned char *prop;
> +	struct platform_device *bram_pdev;
> +	struct device_node *bram_dev;
> +	struct resource *res; /* IO mem resources */
> +	int ret = 0;

Doesn't need to be initialized.

> +	int count = 0;
> +	struct mb_rproc_pdata *local;
> +
> +	local = devm_kzalloc(&pdev->dev, sizeof(*local), GFP_KERNEL);
> +	if (!local)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, local);
> +
> +	/* Declare memory for firmware */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "invalid address\n");
> +		return -ENODEV;
> +	}
> +
> +	local->mem_start = res->start;
> +	local->mem_end = res->end;
> +
> +	/* Alloc phys addr from 0 to max_addr for firmware */
> +	ret = dma_declare_coherent_memory(&pdev->dev, local->mem_start,
> +		local->mem_start, local->mem_end - local->mem_start + 1,
> +		DMA_MEMORY_IO);

This appears to be useless, as you never use dma_alloc_coherent()?

> +	if (!ret) {
> +		dev_err(&pdev->dev, "dma_declare_coherent_memory failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(&pdev->dev, "dma_set_coherent_mask: %d\n", ret);
> +		goto dma_mask_fault;
> +	}
> +
> +	/* Alloc IRQ based on DTS to be sure that no other driver will use it */
> +	while (1) {
> +		int irq;
> +		/* Allocating shared IRQs will ensure that any module will
> +		 * use these IRQs */
> +		irq = platform_get_irq(pdev, count++);
> +		if (irq == -ENXIO || irq == -EINVAL)
> +			break;
> +		ret = devm_request_irq(&pdev->dev, irq, mb_remoteproc_interrupt,
> +				       0, dev_name(&pdev->dev), &pdev->dev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "IRQ %d already allocated\n", irq);
> +			goto dma_mask_fault;
> +		}
> +
> +		dev_info(&pdev->dev, "%d: Alloc irq: %d\n", count, irq);
> +	}

So, you're registering a NOP handler for any of the specified IRQs.
This seems really bizarre.

> +
> +	/* Find out reset gpio and keep microblaze in reset */
> +	local->reset_gpio = of_get_named_gpio(pdev->dev.of_node,
> +					      "reset-gpio", 0);
> +	if (local->reset_gpio < 0) {
> +		dev_err(&pdev->dev, "reset-gpio property not found\n");
> +		ret = local->reset_gpio;
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_gpio_request_one(&pdev->dev, local->reset_gpio,
> +				    GPIOF_OUT_INIT_HIGH, "mb_reset");
> +	if (ret) {
> +		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
> +		goto dma_mask_fault;
> +	}
> +
> +	/* Find out reset gpio and keep microblaze in reset */
> +	local->mb_debug_gpio = of_get_named_gpio(pdev->dev.of_node,
> +						 "debug-gpio", 0);
> +	if (local->mb_debug_gpio < 0) {
> +		dev_err(&pdev->dev, "mb-debug-gpio property not found\n");
> +		ret = local->mb_debug_gpio;
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_gpio_request_one(&pdev->dev, local->mb_debug_gpio,
> +				    GPIOF_OUT_INIT_LOW, "mb_debug");

devm_gpiod_get() might make your life a little easier.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Please specify gpio debug pin\n");
> +		goto dma_mask_fault;
> +	}
> +
> +	/* IPI number for getting irq from firmware */
> +	local->ipi = of_get_named_gpio(pdev->dev.of_node, "ipino-gpio", 0);
> +	if (local->ipi < 0) {
> +		dev_err(&pdev->dev, "ipi-gpio property not found\n");
> +		ret = local->ipi;
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_gpio_request_one(&pdev->dev, local->ipi, GPIOF_IN, "mb_ipi");
> +	if (ret) {
> +		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_request_irq(&pdev->dev, gpio_to_irq(local->ipi),
> +			       ipi_kick, IRQF_SHARED|IRQF_TRIGGER_RISING,
> +			       dev_name(&pdev->dev), local);
> +	if (ret) {
> +		dev_err(&pdev->dev, "IRQ %d already allocated\n", local->ipi);
> +		goto dma_mask_fault;
> +	}
> +
[..]
> +	res = platform_get_resource(bram_pdev, IORESOURCE_MEM, 0);
> +	local->vbase = devm_ioremap_resource(&pdev->dev, res);
> +	if (!local->vbase) {
> +		ret = -ENODEV;
> +		goto dma_mask_fault;
> +	}
> +
> +	/* Load simple bootloader to bram */
> +	local->bootloader = of_get_property(pdev->dev.of_node,
> +					    "bram-firmware", NULL);
> +	if (!local->bootloader) {
> +		dev_err(&pdev->dev, "Please specify BRAM firmware\n");
> +		ret = -ENODEV;
> +		goto dma_mask_fault;
> +	}

Is there precedent for putting firmware file names in a device tree
binary?  This makes me nervous.  Have you considered embedding the
firmware image directly in a binary "bram-firmware" property instead?

> +	dev_info(&pdev->dev, "Using microblaze BRAM bootloader: %s\n",
> +		 local->bootloader);
> +
> +	/* Module param firmware first */
> +	if (firmware)
> +		prop = firmware;
> +	else
> +		prop = of_get_property(pdev->dev.of_node, "firmware", NULL);
> +
> +	if (prop) {

Nit: You're inconsistent style-wise here.  It makes it hard to follow
the "happy path".  Consider:

	if (!prop) {
		ret = -ENODEV;
		goto dma_mask_fault;
	}

> +		dev_info(&pdev->dev, "Using firmware: %s\n", prop);
> +		local->rproc = rproc_alloc(&pdev->dev, dev_name(&pdev->dev),
> +				&mb_rproc_ops, prop, sizeof(struct rproc));
> +		if (!local->rproc) {
> +			dev_err(&pdev->dev, "rproc allocation failed\n");
> +			ret = -ENODEV;
> +			goto dma_mask_fault;
> +		}
> +
> +		ret = rproc_add(local->rproc);
> +		if (ret) {
> +			dev_err(&pdev->dev, "rproc registration failed\n");
> +			rproc_put(local->rproc);
> +			goto dma_mask_fault;
> +		}
> +		return 0;
> +	}
> +
> +	ret = -ENODEV;
> +
> +dma_mask_fault:
> +	dma_release_declared_memory(&pdev->dev);

Does this not get done automatically?

> +
> +	return ret;
> +}
> +
> +static int mb_remoteproc_remove(struct platform_device *pdev)
> +{
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev, "%s\n", __func__);

This is just noise.

> +
> +	dma_release_declared_memory(&pdev->dev);
> +
> +	rproc_del(local->rproc);
> +	rproc_put(local->rproc);
> +
> +	return 0;
> +}
> +
> +/* Match table for OF platform binding */
> +static const struct of_device_id mb_remoteproc_match[] = {
> +	{ .compatible = "xlnx,mb-remoteproc", },
> +	{ /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, mb_remoteproc_match);
> +
> +static struct platform_driver mb_remoteproc_driver = {
> +	.probe = mb_remoteproc_probe,
> +	.remove = mb_remoteproc_remove,
> +	.driver = {
> +		.name = "mb-remoteproc",
> +		.of_match_table = mb_remoteproc_match,
> +	},
> +};
> +module_platform_driver(mb_remoteproc_driver);
> +
> +module_param(firmware, charp, 0);
> +MODULE_PARM_DESC(firmware, "Override the firmware image name. Default value in DTS.");

Two higher level things:

Conceptually, it'd be possible (given sufficient resources) to
instantiate two instances of this MB <-> GPIO <-> BRAM stack.  The way
the driver currently written, this is not possible.  This seems fixable.

The use of BRAM + GPIO really smells like it would fit into the mailbox
framework.  Have you considered this?

  Josh

[PATCH v2 2/2] remoteproc: microblaze: Add support for microblaze on Zynq

[Josh Cartwright]

Hey Michal-

I've got a few comments on this.

On Mon, Feb 23, 2015 at 12:37:26PM +0100, Michal Simek wrote:
> Xilinx Microblaze is placed in programmable logic on Xilinx
> Zynq architecture. Driver requires specific HW setting
> described in DT binding.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
[..]
> +++ b/drivers/remoteproc/Kconfig
> @@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
>  	  It's safe to say n here if you're not interested in multimedia
>  	  offloading.
>  
> +config MB_REMOTEPROC
> +	tristate "Support Microblaze remoteproc"
> +	depends on ARCH_ZYNQ
> +	select GPIO_XILINX

Why?  The driver doesn't (and shouldn't) know which GPIO controller is
being used.

> +	select REMOTEPROC
> +	select RPMSG
> +	default m
> +	help
> +	  Say y here to support Xilinx Microblaze remote processors
> +	  on the Xilinx Zynq.
> +
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ac2ff75686d2..40e247ffac34 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -10,3 +10,4 @@ remoteproc-y				+= remoteproc_elf_loader.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
> +obj-$(CONFIG_MB_REMOTEPROC)		+= mb_remoteproc.o
> diff --git a/drivers/remoteproc/mb_remoteproc.c b/drivers/remoteproc/mb_remoteproc.c
> new file mode 100644
> index 000000000000..7febf32abafc
> --- /dev/null
> +++ b/drivers/remoteproc/mb_remoteproc.c
[..]
> +/* Private data */
> +struct mb_rproc_pdata {
> +	struct rproc *rproc;
> +	u32 mem_start;
> +	u32 mem_end;
> +	int reset_gpio;
> +	int mb_debug_gpio;
> +	int ipi;
> +	int vring0;
> +	int vring1;
> +	void __iomem *vbase;
> +	const unsigned char *bootloader;
> +};
> +
> +/* Store rproc for IPI handler */
> +static struct platform_device *remoteprocdev;
> +static struct work_struct workqueue;

Roll the workqueue into mb_rproc_pdata?  I see no reason that you need
this global state.

> +static void handle_event(struct work_struct *work)
> +{
> +	struct mb_rproc_pdata *local = platform_get_drvdata(remoteprocdev);
> +
> +	flush_cache_all();
> +	outer_flush_range(local->mem_start, local->mem_end);
> +
> +	if (rproc_vq_interrupt(local->rproc, 0) == IRQ_NONE)
> +		dev_info(&remoteprocdev->dev, "no message found in vqid 0\n");
> +}
> +
> +static irqreturn_t ipi_kick(int irq, void *dev_id)
> +{
> +	dev_dbg(&remoteprocdev->dev, "KICK Linux because of pending message\n");
> +	schedule_work(&workqueue);

Perhaps it's easier to use request_threaded_irq() as your mechanism of
deferring instead of a workqueue.

> +	dev_dbg(&remoteprocdev->dev, "KICK Linux handled\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mb_rproc_start(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +	const struct firmware *fw;
> +	int ret;
> +
> +	dev_info(dev, "%s\n", __func__);
> +	INIT_WORK(&workqueue, handle_event);
> +
> +	flush_cache_all();
> +	outer_flush_range(local->mem_start, local->mem_end);
> +
> +	remoteprocdev = pdev;
> +
> +	ret = request_firmware(&fw, local->bootloader, &pdev->dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "request_firmware failed\n");
> +		return ret;
> +	}
> +	/* Copy bootloader to memory */
> +	memcpy(local->vbase, fw->data, fw->size);

sparse is not going to like you.  How are you guaranteeing coherence?

> +	release_firmware(fw);
> +
> +	/* Just for sure synchronize memories */
> +	dsb();
> +
> +	/* Release Microblaze from reset */
> +	gpio_set_value(local->reset_gpio, 0);
> +
> +	return 0;
> +}
> +
> +/* kick a firmware */
> +static void mb_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +
> +	dev_dbg(dev, "KICK Firmware to start send messages vqid %d\n", vqid);
> +
> +	flush_cache_all();
> +	outer_flush_all();
> +
> +	/* Send swirq to firmware */
> +	gpio_set_value(local->vring0, 0);
> +	gpio_set_value(local->vring1, 0);
> +	dsb();
> +
> +	if (!vqid) {
> +		udelay(500);
> +		gpio_set_value(local->vring0, 1);
> +		dsb();
> +	} else {
> +		udelay(100);
> +		gpio_set_value(local->vring1, 1);
> +		dsb();
> +	}
> +}
> +
> +/* power off the remote processor */
> +static int mb_rproc_stop(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +
> +	/* Setup MB to the state where all memory transactions are done */
> +	gpio_set_value(local->mb_debug_gpio, 1);
> +	dsb(); /* Be sure that this write has been done */
> +	/*
> +	 * This should be enough to ensure one CLK as
> +	 * it is written in MB ref guide
> +	 */
> +	gpio_set_value(local->mb_debug_gpio, 0);
> +
> +	udelay(1000); /* Wait some time to finish all mem transactions */
> +
> +	/* Add Microblaze to reset state */
> +	gpio_set_value(local->reset_gpio, 1);
> +
> +	/* No reason to wait that operations where done */
> +	return 0;
> +}
> +
> +static struct rproc_ops mb_rproc_ops = {

const?

> +	.start		= mb_rproc_start,
> +	.stop		= mb_rproc_stop,
> +	.kick		= mb_rproc_kick,
> +};
> +
> +/* Just to detect bug if interrupt forwarding is broken */
> +static irqreturn_t mb_remoteproc_interrupt(int irq, void *dev_id)
> +{
> +	struct device *dev = dev_id;
> +
> +	dev_err(dev, "GIC IRQ %d is not forwarded correctly\n", irq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mb_remoteproc_probe(struct platform_device *pdev)
> +{
> +	const unsigned char *prop;
> +	struct platform_device *bram_pdev;
> +	struct device_node *bram_dev;
> +	struct resource *res; /* IO mem resources */
> +	int ret = 0;

Doesn't need to be initialized.

> +	int count = 0;
> +	struct mb_rproc_pdata *local;
> +
> +	local = devm_kzalloc(&pdev->dev, sizeof(*local), GFP_KERNEL);
> +	if (!local)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, local);
> +
> +	/* Declare memory for firmware */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "invalid address\n");
> +		return -ENODEV;
> +	}
> +
> +	local->mem_start = res->start;
> +	local->mem_end = res->end;
> +
> +	/* Alloc phys addr from 0 to max_addr for firmware */
> +	ret = dma_declare_coherent_memory(&pdev->dev, local->mem_start,
> +		local->mem_start, local->mem_end - local->mem_start + 1,
> +		DMA_MEMORY_IO);

This appears to be useless, as you never use dma_alloc_coherent()?

> +	if (!ret) {
> +		dev_err(&pdev->dev, "dma_declare_coherent_memory failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(&pdev->dev, "dma_set_coherent_mask: %d\n", ret);
> +		goto dma_mask_fault;
> +	}
> +
> +	/* Alloc IRQ based on DTS to be sure that no other driver will use it */
> +	while (1) {
> +		int irq;
> +		/* Allocating shared IRQs will ensure that any module will
> +		 * use these IRQs */
> +		irq = platform_get_irq(pdev, count++);
> +		if (irq == -ENXIO || irq == -EINVAL)
> +			break;
> +		ret = devm_request_irq(&pdev->dev, irq, mb_remoteproc_interrupt,
> +				       0, dev_name(&pdev->dev), &pdev->dev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "IRQ %d already allocated\n", irq);
> +			goto dma_mask_fault;
> +		}
> +
> +		dev_info(&pdev->dev, "%d: Alloc irq: %d\n", count, irq);
> +	}

So, you're registering a NOP handler for any of the specified IRQs.
This seems really bizarre.

> +
> +	/* Find out reset gpio and keep microblaze in reset */
> +	local->reset_gpio = of_get_named_gpio(pdev->dev.of_node,
> +					      "reset-gpio", 0);
> +	if (local->reset_gpio < 0) {
> +		dev_err(&pdev->dev, "reset-gpio property not found\n");
> +		ret = local->reset_gpio;
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_gpio_request_one(&pdev->dev, local->reset_gpio,
> +				    GPIOF_OUT_INIT_HIGH, "mb_reset");
> +	if (ret) {
> +		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
> +		goto dma_mask_fault;
> +	}
> +
> +	/* Find out reset gpio and keep microblaze in reset */
> +	local->mb_debug_gpio = of_get_named_gpio(pdev->dev.of_node,
> +						 "debug-gpio", 0);
> +	if (local->mb_debug_gpio < 0) {
> +		dev_err(&pdev->dev, "mb-debug-gpio property not found\n");
> +		ret = local->mb_debug_gpio;
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_gpio_request_one(&pdev->dev, local->mb_debug_gpio,
> +				    GPIOF_OUT_INIT_LOW, "mb_debug");

devm_gpiod_get() might make your life a little easier.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Please specify gpio debug pin\n");
> +		goto dma_mask_fault;
> +	}
> +
> +	/* IPI number for getting irq from firmware */
> +	local->ipi = of_get_named_gpio(pdev->dev.of_node, "ipino-gpio", 0);
> +	if (local->ipi < 0) {
> +		dev_err(&pdev->dev, "ipi-gpio property not found\n");
> +		ret = local->ipi;
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_gpio_request_one(&pdev->dev, local->ipi, GPIOF_IN, "mb_ipi");
> +	if (ret) {
> +		dev_err(&pdev->dev, "Please specify gpio reset addr\n");
> +		goto dma_mask_fault;
> +	}
> +	ret = devm_request_irq(&pdev->dev, gpio_to_irq(local->ipi),
> +			       ipi_kick, IRQF_SHARED|IRQF_TRIGGER_RISING,
> +			       dev_name(&pdev->dev), local);
> +	if (ret) {
> +		dev_err(&pdev->dev, "IRQ %d already allocated\n", local->ipi);
> +		goto dma_mask_fault;
> +	}
> +
[..]
> +	res = platform_get_resource(bram_pdev, IORESOURCE_MEM, 0);
> +	local->vbase = devm_ioremap_resource(&pdev->dev, res);
> +	if (!local->vbase) {
> +		ret = -ENODEV;
> +		goto dma_mask_fault;
> +	}
> +
> +	/* Load simple bootloader to bram */
> +	local->bootloader = of_get_property(pdev->dev.of_node,
> +					    "bram-firmware", NULL);
> +	if (!local->bootloader) {
> +		dev_err(&pdev->dev, "Please specify BRAM firmware\n");
> +		ret = -ENODEV;
> +		goto dma_mask_fault;
> +	}

Is there precedent for putting firmware file names in a device tree
binary?  This makes me nervous.  Have you considered embedding the
firmware image directly in a binary "bram-firmware" property instead?

> +	dev_info(&pdev->dev, "Using microblaze BRAM bootloader: %s\n",
> +		 local->bootloader);
> +
> +	/* Module param firmware first */
> +	if (firmware)
> +		prop = firmware;
> +	else
> +		prop = of_get_property(pdev->dev.of_node, "firmware", NULL);
> +
> +	if (prop) {

Nit: You're inconsistent style-wise here.  It makes it hard to follow
the "happy path".  Consider:

	if (!prop) {
		ret = -ENODEV;
		goto dma_mask_fault;
	}

> +		dev_info(&pdev->dev, "Using firmware: %s\n", prop);
> +		local->rproc = rproc_alloc(&pdev->dev, dev_name(&pdev->dev),
> +				&mb_rproc_ops, prop, sizeof(struct rproc));
> +		if (!local->rproc) {
> +			dev_err(&pdev->dev, "rproc allocation failed\n");
> +			ret = -ENODEV;
> +			goto dma_mask_fault;
> +		}
> +
> +		ret = rproc_add(local->rproc);
> +		if (ret) {
> +			dev_err(&pdev->dev, "rproc registration failed\n");
> +			rproc_put(local->rproc);
> +			goto dma_mask_fault;
> +		}
> +		return 0;
> +	}
> +
> +	ret = -ENODEV;
> +
> +dma_mask_fault:
> +	dma_release_declared_memory(&pdev->dev);

Does this not get done automatically?

> +
> +	return ret;
> +}
> +
> +static int mb_remoteproc_remove(struct platform_device *pdev)
> +{
> +	struct mb_rproc_pdata *local = platform_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev, "%s\n", __func__);

This is just noise.

> +
> +	dma_release_declared_memory(&pdev->dev);
> +
> +	rproc_del(local->rproc);
> +	rproc_put(local->rproc);
> +
> +	return 0;
> +}
> +
> +/* Match table for OF platform binding */
> +static const struct of_device_id mb_remoteproc_match[] = {
> +	{ .compatible = "xlnx,mb-remoteproc", },
> +	{ /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, mb_remoteproc_match);
> +
> +static struct platform_driver mb_remoteproc_driver = {
> +	.probe = mb_remoteproc_probe,
> +	.remove = mb_remoteproc_remove,
> +	.driver = {
> +		.name = "mb-remoteproc",
> +		.of_match_table = mb_remoteproc_match,
> +	},
> +};
> +module_platform_driver(mb_remoteproc_driver);
> +
> +module_param(firmware, charp, 0);
> +MODULE_PARM_DESC(firmware, "Override the firmware image name. Default value in DTS.");

Two higher level things:

Conceptually, it'd be possible (given sufficient resources) to
instantiate two instances of this MB <-> GPIO <-> BRAM stack.  The way
the driver currently written, this is not possible.  This seems fixable.

The use of BRAM + GPIO really smells like it would fit into the mailbox
framework.  Have you considered this?

  Josh