diff mbox series

[U-Boot,v2,03/13] mailbox: zynqmp: ipi mailbox driver

Message ID 66675b4b3d42078b62c8fa1235e88165d83cb790.1570023563.git.michal.simek@xilinx.com
State Accepted
Commit 660b0c77d81603c7911a3e5c024d646a801cd0ac
Delegated to: Michal Simek
Headers show
Series arm64: zynqmp: Clean communication with PMUFW | expand

Commit Message

Michal Simek Oct. 2, 2019, 1:39 p.m. UTC 
From: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com>

ZynqMP mailbox driver implementing IPI communication with PMU. This would
allow U-Boot SPL to communicate with PMUFW to request privileged
operations.

Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2: None

 MAINTAINERS                                   |   1 +
 arch/arm/mach-zynqmp/include/mach/sys_proto.h |   5 +
 drivers/mailbox/Kconfig                       |   6 +
 drivers/mailbox/Makefile                      |   1 +
 drivers/mailbox/zynqmp-ipi.c                  | 134 ++++++++++++++++++
 5 files changed, 147 insertions(+)
 create mode 100644 drivers/mailbox/zynqmp-ipi.c

Comments

Luca Ceresoli Oct. 9, 2019, 3:02 p.m. UTC | #1 
Hi Ibai, Michal,

I had half-written a review of this patch and patch 4. Unfortunately I
didn't finish them before they got applied. I'll send them now anyway,
they are mostly nitpicking but you might consider them for a future
improvement. Sorry for the inconvenience.


On 02/10/19 15:39, Michal Simek wrote:
> From: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com>
> 
> ZynqMP mailbox driver implementing IPI communication with PMU. This would
> allow U-Boot SPL to communicate with PMUFW to request privileged
> operations.
> 
> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

...

> +static int zynqmp_ipi_probe(struct udevice *dev)
> +{
> +	struct zynqmp_ipi *zynqmp = dev_get_priv(dev);
> +	struct resource res;
> +	ofnode node;
> +
> +	debug("%s(dev=%p)\n", __func__, dev);
> +
> +	/* Get subnode where the regs are defined */
> +	/* Note IPI mailbox node needs to be the first one in DT */
> +	node = ofnode_first_subnode(dev_ofnode(dev));
> +
> +	if (ofnode_read_resource_byname(node, "local_request_region", &res)) {
> +		dev_err(dev, "No reg property for local_request_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->local_req_regs = devm_ioremap(dev, res.start,
> +					      (res.start - res.end));
> +
> +	if (ofnode_read_resource_byname(node, "local_response_region", &res)) {
> +		dev_err(dev, "No reg property for local_response_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->local_res_regs = devm_ioremap(dev, res.start,
> +					      (res.start - res.end));
> +
> +	if (ofnode_read_resource_byname(node, "remote_request_region", &res)) {
> +		dev_err(dev, "No reg property for remote_request_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->remote_req_regs = devm_ioremap(dev, res.start,
> +					       (res.start - res.end));
> +
> +	if (ofnode_read_resource_byname(node, "remote_response_region", &res)) {
> +		dev_err(dev, "No reg property for remote_response_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->remote_res_regs = devm_ioremap(dev, res.start,
> +					       (res.start - res.end));

remote_req_regs and remote_res_regs are not used, why adding them in DT?

Should there be a good reason to keep them, I think the above code could
be reorganized to avoid code duplication (assuming binary size of a
bootloader still matters nowadays).
Michal Simek Oct. 10, 2019, 9:21 a.m. UTC | #2 
On 09. 10. 19 17:02, Luca Ceresoli wrote:
> Hi Ibai, Michal,
> 
> I had half-written a review of this patch and patch 4. Unfortunately I
> didn't finish them before they got applied. I'll send them now anyway,
> they are mostly nitpicking but you might consider them for a future
> improvement. Sorry for the inconvenience.
> 
> 
> On 02/10/19 15:39, Michal Simek wrote:
>> From: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com>
>>
>> ZynqMP mailbox driver implementing IPI communication with PMU. This would
>> allow U-Boot SPL to communicate with PMUFW to request privileged
>> operations.
>>
>> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> 
> ...
> 
>> +static int zynqmp_ipi_probe(struct udevice *dev)
>> +{
>> +	struct zynqmp_ipi *zynqmp = dev_get_priv(dev);
>> +	struct resource res;
>> +	ofnode node;
>> +
>> +	debug("%s(dev=%p)\n", __func__, dev);
>> +
>> +	/* Get subnode where the regs are defined */
>> +	/* Note IPI mailbox node needs to be the first one in DT */
>> +	node = ofnode_first_subnode(dev_ofnode(dev));
>> +
>> +	if (ofnode_read_resource_byname(node, "local_request_region", &res)) {
>> +		dev_err(dev, "No reg property for local_request_region\n");
>> +		return -EINVAL;
>> +	};
>> +	zynqmp->local_req_regs = devm_ioremap(dev, res.start,
>> +					      (res.start - res.end));
>> +
>> +	if (ofnode_read_resource_byname(node, "local_response_region", &res)) {
>> +		dev_err(dev, "No reg property for local_response_region\n");
>> +		return -EINVAL;
>> +	};
>> +	zynqmp->local_res_regs = devm_ioremap(dev, res.start,
>> +					      (res.start - res.end));
>> +
>> +	if (ofnode_read_resource_byname(node, "remote_request_region", &res)) {
>> +		dev_err(dev, "No reg property for remote_request_region\n");
>> +		return -EINVAL;
>> +	};
>> +	zynqmp->remote_req_regs = devm_ioremap(dev, res.start,
>> +					       (res.start - res.end));
>> +
>> +	if (ofnode_read_resource_byname(node, "remote_response_region", &res)) {
>> +		dev_err(dev, "No reg property for remote_response_region\n");
>> +		return -EINVAL;
>> +	};
>> +	zynqmp->remote_res_regs = devm_ioremap(dev, res.start,
>> +					       (res.start - res.end));
> 
> remote_req_regs and remote_res_regs are not used, why adding them in DT?
> 
> Should there be a good reason to keep them, I think the above code could
> be reorganized to avoid code duplication (assuming binary size of a
> bootloader still matters nowadays).
> 

Binding is taken from Linux kernel and these are required property
there. I think it is used by Linux driver. It means checking required
property is good thing to do.

Regarding if make sense to map them if they are not used. I think we can
remove this code.

If make sense reorganized the code to make it smaller. Sure of course.
Patches welcome.

Thanks,
Michal
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index f448c5f19e00..f5feb89ac3e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -448,6 +448,7 @@  F:	drivers/gpio/zynq_gpio.c
 F:	drivers/i2c/i2c-cdns.c
 F:	drivers/i2c/muxes/pca954x.c
 F:	drivers/i2c/zynq_i2c.c
+F:	drivers/mailbox/zynqmp-ipi.c
 F:	drivers/mmc/zynq_sdhci.c
 F:	drivers/mtd/nand/raw/zynq_nand.c
 F:	drivers/net/phy/xilinx_phy.c
diff --git a/arch/arm/mach-zynqmp/include/mach/sys_proto.h b/arch/arm/mach-zynqmp/include/mach/sys_proto.h
index 915badc6fbee..f25d414dcb1e 100644
--- a/arch/arm/mach-zynqmp/include/mach/sys_proto.h
+++ b/arch/arm/mach-zynqmp/include/mach/sys_proto.h
@@ -54,6 +54,11 @@  enum {
 	TCM_SPLIT,
 };
 
+struct zynqmp_ipi_msg {
+	size_t len;
+	u32 *buf;
+};
+
 int zynq_board_read_rom_ethaddr(unsigned char *ethaddr);
 unsigned int zynqmp_get_silicon_version(void);
 
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 11bf5522db53..85c2a829aed8 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -41,4 +41,10 @@  config K3_SEC_PROXY
 	  Select this driver if your platform has support for this hardware
 	  block.
 
+config ZYNQMP_IPI
+	bool "Xilinx ZynqMP IPI controller support"
+	depends on DM_MAILBOX && ARCH_ZYNQMP
+	help
+	  This enables support for the Xilinx ZynqMP Inter Processor Interrupt
+	  communication controller.
 endmenu
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index a753cc4e6806..d2ace8cd212e 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -9,3 +9,4 @@  obj-$(CONFIG_SANDBOX_MBOX) += sandbox-mbox-test.o
 obj-$(CONFIG_STM32_IPCC) += stm32-ipcc.o
 obj-$(CONFIG_TEGRA_HSP) += tegra-hsp.o
 obj-$(CONFIG_K3_SEC_PROXY) += k3-sec-proxy.o
+obj-$(CONFIG_ZYNQMP_IPI) += zynqmp-ipi.o
diff --git a/drivers/mailbox/zynqmp-ipi.c b/drivers/mailbox/zynqmp-ipi.c
new file mode 100644
index 000000000000..c181a7b81768
--- /dev/null
+++ b/drivers/mailbox/zynqmp-ipi.c
@@ -0,0 +1,134 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Xilinx Zynq MPSoC Mailbox driver
+ *
+ * Copyright (C) 2018-2019 Xilinx, Inc.
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <dm.h>
+#include <mailbox-uclass.h>
+#include <mach/sys_proto.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <wait_bit.h>
+
+/* IPI bitmasks, register base */
+/* TODO: move reg base to DT */
+#define IPI_BIT_MASK_PMU0     0x10000
+#define IPI_INT_REG_BASE_APU  0xFF300000
+
+struct ipi_int_regs {
+	u32 trig; /* 0x0  */
+	u32 obs;  /* 0x4  */
+	u32 ist;  /* 0x8  */
+	u32 imr;  /* 0xC  */
+	u32 ier;  /* 0x10 */
+	u32 idr;  /* 0x14 */
+};
+
+#define ipi_int_apu ((struct ipi_int_regs *)IPI_INT_REG_BASE_APU)
+
+struct zynqmp_ipi {
+	void __iomem *local_req_regs;
+	void __iomem *local_res_regs;
+	void __iomem *remote_req_regs;
+	void __iomem *remote_res_regs;
+};
+
+static int zynqmp_ipi_send(struct mbox_chan *chan, const void *data)
+{
+	const struct zynqmp_ipi_msg *msg = (struct zynqmp_ipi_msg *)data;
+	struct zynqmp_ipi *zynqmp = dev_get_priv(chan->dev);
+	u32 ret;
+	u32 *mbx = (u32 *)zynqmp->local_req_regs;
+
+	for (size_t i = 0; i < msg->len; i++)
+		writel(msg->buf[i], &mbx[i]);
+
+	/* Write trigger interrupt */
+	writel(IPI_BIT_MASK_PMU0, &ipi_int_apu->trig);
+
+	/* Wait until observation bit is cleared */
+	ret = wait_for_bit_le32(&ipi_int_apu->obs, IPI_BIT_MASK_PMU0, false,
+				100, false);
+
+	debug("%s, send %ld bytes\n", __func__, msg->len);
+	return ret;
+};
+
+static int zynqmp_ipi_recv(struct mbox_chan *chan, void *data)
+{
+	struct zynqmp_ipi_msg *msg = (struct zynqmp_ipi_msg *)data;
+	struct zynqmp_ipi *zynqmp = dev_get_priv(chan->dev);
+	u32 *mbx = (u32 *)zynqmp->local_res_regs;
+
+	for (size_t i = 0; i < msg->len; i++)
+		msg->buf[i] = readl(&mbx[i]);
+
+	debug("%s, recv %ld bytes\n", __func__, msg->len);
+	return 0;
+};
+
+static int zynqmp_ipi_probe(struct udevice *dev)
+{
+	struct zynqmp_ipi *zynqmp = dev_get_priv(dev);
+	struct resource res;
+	ofnode node;
+
+	debug("%s(dev=%p)\n", __func__, dev);
+
+	/* Get subnode where the regs are defined */
+	/* Note IPI mailbox node needs to be the first one in DT */
+	node = ofnode_first_subnode(dev_ofnode(dev));
+
+	if (ofnode_read_resource_byname(node, "local_request_region", &res)) {
+		dev_err(dev, "No reg property for local_request_region\n");
+		return -EINVAL;
+	};
+	zynqmp->local_req_regs = devm_ioremap(dev, res.start,
+					      (res.start - res.end));
+
+	if (ofnode_read_resource_byname(node, "local_response_region", &res)) {
+		dev_err(dev, "No reg property for local_response_region\n");
+		return -EINVAL;
+	};
+	zynqmp->local_res_regs = devm_ioremap(dev, res.start,
+					      (res.start - res.end));
+
+	if (ofnode_read_resource_byname(node, "remote_request_region", &res)) {
+		dev_err(dev, "No reg property for remote_request_region\n");
+		return -EINVAL;
+	};
+	zynqmp->remote_req_regs = devm_ioremap(dev, res.start,
+					       (res.start - res.end));
+
+	if (ofnode_read_resource_byname(node, "remote_response_region", &res)) {
+		dev_err(dev, "No reg property for remote_response_region\n");
+		return -EINVAL;
+	};
+	zynqmp->remote_res_regs = devm_ioremap(dev, res.start,
+					       (res.start - res.end));
+
+	return 0;
+};
+
+static const struct udevice_id zynqmp_ipi_ids[] = {
+	{ .compatible = "xlnx,zynqmp-ipi-mailbox" },
+	{ }
+};
+
+struct mbox_ops zynqmp_ipi_mbox_ops = {
+	.send = zynqmp_ipi_send,
+	.recv = zynqmp_ipi_recv,
+};
+
+U_BOOT_DRIVER(zynqmp_ipi) = {
+	.name = "zynqmp-ipi",
+	.id = UCLASS_MAILBOX,
+	.of_match = zynqmp_ipi_ids,
+	.probe = zynqmp_ipi_probe,
+	.priv_auto_alloc_size = sizeof(struct zynqmp_ipi),
+	.ops = &zynqmp_ipi_mbox_ops,
+};