| Message ID | 1569824287-4263-3-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mailbox: arm: introduce smc triggered mailbox | expand |
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.
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.
> 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.
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_ */