[V10,2/2] mailbox: introduce ARM SMC based mailbox
diff mbox series

Message ID 1569824287-4263-3-git-send-email-peng.fan@nxp.com
State New
Headers show
Series
  • mailbox: arm: introduce smc triggered mailbox
Related show

Commit Message

Peng Fan Sept. 30, 2019, 6:20 a.m. UTC
From: Peng Fan <peng.fan@nxp.com>

This mailbox driver implements a mailbox which signals transmitted data
via an ARM smc (secure monitor call) instruction. The mailbox receiver
is implemented in firmware and can synchronously return data when it
returns execution to the non-secure world again.
An asynchronous receive path is not implemented.
This allows the usage of a mailbox to trigger firmware actions on SoCs
which either don't have a separate management processor or on which such
a core is not available. A user of this mailbox could be the SCP
interface.

Modified from Andre Przywara's v2 patch
https://lore.kernel.org/patchwork/patch/812999/

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/mailbox/Kconfig                |   7 ++
 drivers/mailbox/Makefile               |   2 +
 drivers/mailbox/arm-smc-mailbox.c      | 166 +++++++++++++++++++++++++++++++++
 include/linux/mailbox/arm-smccc-mbox.h |  20 ++++
 4 files changed, 195 insertions(+)
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c
 create mode 100644 include/linux/mailbox/arm-smccc-mbox.h

Comments

Florian Fainelli Nov. 8, 2019, 5:32 p.m. UTC | #1
Hi Peng,

On 9/29/19 11:20 PM, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> This mailbox driver implements a mailbox which signals transmitted data
> via an ARM smc (secure monitor call) instruction. The mailbox receiver
> is implemented in firmware and can synchronously return data when it
> returns execution to the non-secure world again.
> An asynchronous receive path is not implemented.
> This allows the usage of a mailbox to trigger firmware actions on SoCs
> which either don't have a separate management processor or on which such
> a core is not available. A user of this mailbox could be the SCP
> interface.

Sorry for not spotting this, or rather asking this earlier, but I do
have one question below.

[snip]

> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> +{
> +	struct arm_smc_chan_data *chan_data = link->con_priv;
> +	struct arm_smccc_mbox_cmd *cmd = data;
> +	unsigned long ret;
> +
> +	if (ARM_SMCCC_IS_64(chan_data->function_id)) {
> +		ret = chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> +						    cmd->args_smccc64[0],
> +						    cmd->args_smccc64[1],
> +						    cmd->args_smccc64[2],
> +						    cmd->args_smccc64[3],
> +						    cmd->args_smccc64[4],
> +						    cmd->args_smccc64[5]);
> +	} else {
> +		ret = chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> +						    cmd->args_smccc32[0],
> +						    cmd->args_smccc32[1],
> +						    cmd->args_smccc32[2],
> +						    cmd->args_smccc32[3],
> +						    cmd->args_smccc32[4],
> +						    cmd->args_smccc32[5]);
> +	}

Why did not we use unsigned long for the args_smccc[] array to be bit
width independent, this is what the PSCI infrastructure does and it
looks a lot nicer IMHO. More question below.

[snip]

> +
> +#ifndef _LINUX_ARM_SMCCC_MBOX_H_
> +#define _LINUX_ARM_SMCCC_MBOX_H_
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct arm_smccc_mbox_cmd - ARM SMCCC message structure
> + * @args_smccc32/64:	actual usage of registers is up to the protocol
> + *			(within the SMCCC limits)
> + */
> +struct arm_smccc_mbox_cmd {
> +	union {
> +		u32 args_smccc32[6];
> +		u64 args_smccc64[6];
> +	};
> +};

Why is this being moved to a separate header file and not within the
driver's main file? It is not like we offer the ability for a driver to
embed this ARM SMC mailbox driver as a library, and customize the values
of the SMC arguments (maybe we should do that, as a later patch) except
for the function_id. If you have a "public" header, there is usually a
service or some configuration that your driver would offer, which is not
the case here.
Andre Przywara Nov. 12, 2019, 11:24 a.m. UTC | #2
On Fri, 8 Nov 2019 09:32:43 -0800
Florian Fainelli <f.fainelli@gmail.com> wrote:

Hi Florian,

> On 9/29/19 11:20 PM, Peng Fan wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > This mailbox driver implements a mailbox which signals transmitted data
> > via an ARM smc (secure monitor call) instruction. The mailbox receiver
> > is implemented in firmware and can synchronously return data when it
> > returns execution to the non-secure world again.
> > An asynchronous receive path is not implemented.
> > This allows the usage of a mailbox to trigger firmware actions on SoCs
> > which either don't have a separate management processor or on which such
> > a core is not available. A user of this mailbox could be the SCP
> > interface.  
> 
> Sorry for not spotting this, or rather asking this earlier, but I do
> have one question below.
> 
> [snip]
> 
> > +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> > +{
> > +	struct arm_smc_chan_data *chan_data = link->con_priv;
> > +	struct arm_smccc_mbox_cmd *cmd = data;
> > +	unsigned long ret;
> > +
> > +	if (ARM_SMCCC_IS_64(chan_data->function_id)) {
> > +		ret = chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> > +						    cmd->args_smccc64[0],
> > +						    cmd->args_smccc64[1],
> > +						    cmd->args_smccc64[2],
> > +						    cmd->args_smccc64[3],
> > +						    cmd->args_smccc64[4],
> > +						    cmd->args_smccc64[5]);
> > +	} else {
> > +		ret = chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> > +						    cmd->args_smccc32[0],
> > +						    cmd->args_smccc32[1],
> > +						    cmd->args_smccc32[2],
> > +						    cmd->args_smccc32[3],
> > +						    cmd->args_smccc32[4],
> > +						    cmd->args_smccc32[5]);
> > +	}  
> 
> Why did not we use unsigned long for the args_smccc[] array to be bit
> width independent, this is what the PSCI infrastructure does and it
> looks a lot nicer IMHO. More question below.

Huh, interestingly I think this comes from the combination of the two problems you point out, which evolved separately:
Earlier we had no exported interface between the transport driver and the mailbox client, just a void pointer. So using "long" in the structure would not work, because it would behave differently between arm32 and arm64 kernels. But the firmware interface would always be fixed to one of the two calling conventions, regardless of the kernel "bitness", as advertised by the upper bits of the function ID.
So we introduced explicit types that are used depending on the firmware-advertised calling convention. The idea was that any packed data any client would provide would always end up in consecutive registers in the firmware.
Now we explicitly advertise the expected message structure in the new header file, so we could go back to unsigned long here, indeed. A 32-bit kernel could never use the 64-bit calling convention, so long would fit. In a 64-bit kernel the compiler would either downgrade the long argument to the 32-bit arguments the firmware expects, or keep it long.
So it might be worth a short to go back to long.

> 
> [snip]
> 
> > +
> > +#ifndef _LINUX_ARM_SMCCC_MBOX_H_
> > +#define _LINUX_ARM_SMCCC_MBOX_H_
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct arm_smccc_mbox_cmd - ARM SMCCC message structure
> > + * @args_smccc32/64:	actual usage of registers is up to the protocol
> > + *			(within the SMCCC limits)
> > + */
> > +struct arm_smccc_mbox_cmd {
> > +	union {
> > +		u32 args_smccc32[6];
> > +		u64 args_smccc64[6];
> > +	};
> > +};  
> 
> Why is this being moved to a separate header file and not within the
> driver's main file? It is not like we offer the ability for a driver to
> embed this ARM SMC mailbox driver as a library, and customize the values
> of the SMC arguments (maybe we should do that, as a later patch) except
> for the function_id.

I wouldn't call it a "library", but indeed we expose the transport protocol to the mailbox client. It seems that the mailbox framework is not really clear here, it just states that (at least in many cases) the mailbox client knows about the transport protocol, even though the separation between the two suggests otherwise. This probably stems back from the days, where mailboxes were directly used by their users, without providing any kind of abstraction.
So going with this, the SMC mailbox transport driver enforces a specific transport protocol for the payload, namely the six SMCCC defined registers. So we make this available, so any mailbox client knows what to expect. At the end of the day on the other end there will be some firmware probably expecting specific data in specific registers - or no data at all, as in the simple doorbell case we intend to use for SCPI/SCMI.

> If you have a "public" header, there is usually a
> service or some configuration that your driver would offer, which is not
> the case here.

If you want to use the mailbox just as a doorbell (as in our case), it doesn't matter, so we can as well expose the underlying transport protocol.

Cheers,
Andre.
Peng Fan Nov. 22, 2019, 9:47 a.m. UTC | #3
> Subject: Re: [PATCH V10 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Fri, 8 Nov 2019 09:32:43 -0800
> Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> Hi Florian,
> 
> > On 9/29/19 11:20 PM, Peng Fan wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > This mailbox driver implements a mailbox which signals transmitted
> > > data via an ARM smc (secure monitor call) instruction. The mailbox
> > > receiver is implemented in firmware and can synchronously return
> > > data when it returns execution to the non-secure world again.
> > > An asynchronous receive path is not implemented.
> > > This allows the usage of a mailbox to trigger firmware actions on
> > > SoCs which either don't have a separate management processor or on
> > > which such a core is not available. A user of this mailbox could be
> > > the SCP interface.
> >
> > Sorry for not spotting this, or rather asking this earlier, but I do
> > have one question below.
> >
> > [snip]
> >
> > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > +	struct arm_smc_chan_data *chan_data = link->con_priv;
> > > +	struct arm_smccc_mbox_cmd *cmd = data;
> > > +	unsigned long ret;
> > > +
> > > +	if (ARM_SMCCC_IS_64(chan_data->function_id)) {
> > > +		ret =
> chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> > > +						    cmd->args_smccc64[0],
> > > +						    cmd->args_smccc64[1],
> > > +						    cmd->args_smccc64[2],
> > > +						    cmd->args_smccc64[3],
> > > +						    cmd->args_smccc64[4],
> > > +						    cmd->args_smccc64[5]);
> > > +	} else {
> > > +		ret =
> chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> > > +						    cmd->args_smccc32[0],
> > > +						    cmd->args_smccc32[1],
> > > +						    cmd->args_smccc32[2],
> > > +						    cmd->args_smccc32[3],
> > > +						    cmd->args_smccc32[4],
> > > +						    cmd->args_smccc32[5]);
> > > +	}
> >
> > Why did not we use unsigned long for the args_smccc[] array to be bit
> > width independent, this is what the PSCI infrastructure does and it
> > looks a lot nicer IMHO. More question below.
> 
> Huh, interestingly I think this comes from the combination of the two
> problems you point out, which evolved separately:
> Earlier we had no exported interface between the transport driver and the
> mailbox client, just a void pointer. So using "long" in the structure would not
> work, because it would behave differently between arm32 and arm64 kernels.
> But the firmware interface would always be fixed to one of the two calling
> conventions, regardless of the kernel "bitness", as advertised by the upper bits
> of the function ID.
> So we introduced explicit types that are used depending on the
> firmware-advertised calling convention. The idea was that any packed data
> any client would provide would always end up in consecutive registers in the
> firmware.
> Now we explicitly advertise the expected message structure in the new
> header file, so we could go back to unsigned long here, indeed. A 32-bit kernel
> could never use the 64-bit calling convention, so long would fit. In a 64-bit
> kernel the compiler would either downgrade the long argument to the 32-bit
> arguments the firmware expects, or keep it long.
> So it might be worth a short to go back to long.

I'll drop the check ARM_SMCCC_IS_64(chan_data->function_id) and
Directly
 chan_data->invoke_smc_mbox_fn(chan_data->function_id,
						    cmd->args_smccc[0],
						    cmd->args_smccc[1],
						    cmd->args_smccc[2],
						    cmd->args_smccc[3],
						    cmd->args_smccc[4],
						    cmd->args_smccc[5]);

Is this ok for you?

> 
> >
> > [snip]
> >
> > > +
> > > +#ifndef _LINUX_ARM_SMCCC_MBOX_H_
> > > +#define _LINUX_ARM_SMCCC_MBOX_H_
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/**
> > > + * struct arm_smccc_mbox_cmd - ARM SMCCC message structure
> > > + * @args_smccc32/64:	actual usage of registers is up to the protocol
> > > + *			(within the SMCCC limits)
> > > + */
> > > +struct arm_smccc_mbox_cmd {
> > > +	union {
> > > +		u32 args_smccc32[6];
> > > +		u64 args_smccc64[6];
> > > +	};
> > > +};
> >
> > Why is this being moved to a separate header file and not within the
> > driver's main file? It is not like we offer the ability for a driver
> > to embed this ARM SMC mailbox driver as a library, and customize the
> > values of the SMC arguments (maybe we should do that, as a later
> > patch) except for the function_id.
> 
> I wouldn't call it a "library", but indeed we expose the transport protocol to
> the mailbox client. It seems that the mailbox framework is not really clear
> here, it just states that (at least in many cases) the mailbox client knows about
> the transport protocol, even though the separation between the two suggests
> otherwise. This probably stems back from the days, where mailboxes were
> directly used by their users, without providing any kind of abstraction.
> So going with this, the SMC mailbox transport driver enforces a specific
> transport protocol for the payload, namely the six SMCCC defined registers. So
> we make this available, so any mailbox client knows what to expect. At the
> end of the day on the other end there will be some firmware probably
> expecting specific data in specific registers - or no data at all, as in the simple
> doorbell case we intend to use for SCPI/SCMI.

struct arm_smccc_mbox_cmd {
	unsigned long args_smccc[6];
};

Is this ok for you?

Thanks,
Peng.


> 
> > If you have a "public" header, there is usually a service or some
> > configuration that your driver would offer, which is not the case
> > here.
> 
> If you want to use the mailbox just as a doorbell (as in our case), it doesn't
> matter, so we can as well expose the underlying transport protocol.
> 
> Cheers,
> Andre.

Patch
diff mbox series

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ab4eb750bbdd..7707ee26251a 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -16,6 +16,13 @@  config ARM_MHU
 	  The controller has 3 mailbox channels, the last of which can be
 	  used in Secure mode only.
 
+config ARM_SMC_MBOX
+	tristate "Generic ARM smc mailbox"
+	depends on OF && HAVE_ARM_SMCCC
+	help
+	  Generic mailbox driver which uses ARM smc calls to call into
+	  firmware for triggering mailboxes.
+
 config IMX_MBOX
 	tristate "i.MX Mailbox"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index c22fad6f696b..93918a84c91b 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,6 +7,8 @@  obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
 
 obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
 
+obj-$(CONFIG_ARM_SMC_MBOX)	+= arm-smc-mailbox.o
+
 obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
 
 obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)	+= armada-37xx-rwtm-mailbox.o
diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
new file mode 100644
index 000000000000..a6ec56f41f7f
--- /dev/null
+++ b/drivers/mailbox/arm-smc-mailbox.c
@@ -0,0 +1,166 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016,2017 ARM Ltd.
+ * Copyright 2019 NXP
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/arm-smccc-mbox.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
+				    unsigned long, unsigned long,
+				    unsigned long, unsigned long,
+				    unsigned long);
+
+struct arm_smc_chan_data {
+	unsigned int function_id;
+	smc_mbox_fn *invoke_smc_mbox_fn;
+};
+
+static int arm_smc_send_data(struct mbox_chan *link, void *data)
+{
+	struct arm_smc_chan_data *chan_data = link->con_priv;
+	struct arm_smccc_mbox_cmd *cmd = data;
+	unsigned long ret;
+
+	if (ARM_SMCCC_IS_64(chan_data->function_id)) {
+		ret = chan_data->invoke_smc_mbox_fn(chan_data->function_id,
+						    cmd->args_smccc64[0],
+						    cmd->args_smccc64[1],
+						    cmd->args_smccc64[2],
+						    cmd->args_smccc64[3],
+						    cmd->args_smccc64[4],
+						    cmd->args_smccc64[5]);
+	} else {
+		ret = chan_data->invoke_smc_mbox_fn(chan_data->function_id,
+						    cmd->args_smccc32[0],
+						    cmd->args_smccc32[1],
+						    cmd->args_smccc32[2],
+						    cmd->args_smccc32[3],
+						    cmd->args_smccc32[4],
+						    cmd->args_smccc32[5]);
+	}
+
+	mbox_chan_received_data(link, (void *)ret);
+
+	return 0;
+}
+
+static unsigned long __invoke_fn_hvc(unsigned int function_id,
+				     unsigned long arg0, unsigned long arg1,
+				     unsigned long arg2, unsigned long arg3,
+				     unsigned long arg4, unsigned long arg5)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
+		      arg5, 0, &res);
+	return res.a0;
+}
+
+static unsigned long __invoke_fn_smc(unsigned int function_id,
+				     unsigned long arg0, unsigned long arg1,
+				     unsigned long arg2, unsigned long arg3,
+				     unsigned long arg4, unsigned long arg5)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
+		      arg5, 0, &res);
+	return res.a0;
+}
+
+static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
+	.send_data	= arm_smc_send_data,
+};
+
+static struct mbox_chan *
+arm_smc_mbox_of_xlate(struct mbox_controller *mbox,
+		      const struct of_phandle_args *sp)
+{
+	return mbox->chans;
+}
+
+static int arm_smc_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mbox_controller *mbox;
+	struct arm_smc_chan_data *chan_data;
+	int ret;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	mbox->of_xlate = arm_smc_mbox_of_xlate;
+	mbox->num_chans = 1;
+	mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans), GFP_KERNEL);
+	if (!mbox->chans)
+		return -ENOMEM;
+
+	chan_data = devm_kzalloc(dev, sizeof(*chan_data), GFP_KERNEL);
+	if (!chan_data)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(dev->of_node, "arm,func-id",
+				   &chan_data->function_id);
+	if (ret)
+		return ret;
+
+	if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
+		chan_data->invoke_smc_mbox_fn = __invoke_fn_smc;
+	else
+		chan_data->invoke_smc_mbox_fn = __invoke_fn_hvc;
+
+
+	mbox->chans->con_priv = chan_data;
+
+	mbox->txdone_poll = false;
+	mbox->txdone_irq = false;
+	mbox->ops = &arm_smc_mbox_chan_ops;
+	mbox->dev = dev;
+
+	platform_set_drvdata(pdev, mbox);
+
+	ret = devm_mbox_controller_register(dev, mbox);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "ARM SMC mailbox enabled.\n");
+
+	return ret;
+}
+
+static int arm_smc_mbox_remove(struct platform_device *pdev)
+{
+	struct mbox_controller *mbox = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(mbox);
+	return 0;
+}
+
+static const struct of_device_id arm_smc_mbox_of_match[] = {
+	{ .compatible = "arm,smc-mbox", },
+	{ .compatible = "arm,hvc-mbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, arm_smc_mbox_of_match);
+
+static struct platform_driver arm_smc_mbox_driver = {
+	.driver = {
+		.name = "arm-smc-mbox",
+		.of_match_table = arm_smc_mbox_of_match,
+	},
+	.probe		= arm_smc_mbox_probe,
+	.remove		= arm_smc_mbox_remove,
+};
+module_platform_driver(arm_smc_mbox_driver);
+
+MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
+MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mailbox/arm-smccc-mbox.h b/include/linux/mailbox/arm-smccc-mbox.h
new file mode 100644
index 000000000000..d35fb89a77f5
--- /dev/null
+++ b/include/linux/mailbox/arm-smccc-mbox.h
@@ -0,0 +1,20 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_ARM_SMCCC_MBOX_H_
+#define _LINUX_ARM_SMCCC_MBOX_H_
+
+#include <linux/types.h>
+
+/**
+ * struct arm_smccc_mbox_cmd - ARM SMCCC message structure
+ * @args_smccc32/64:	actual usage of registers is up to the protocol
+ *			(within the SMCCC limits)
+ */
+struct arm_smccc_mbox_cmd {
+	union {
+		u32 args_smccc32[6];
+		u64 args_smccc64[6];
+	};
+};
+
+#endif /* _LINUX_ARM_SMCCC_MBOX_H_ */