[PATCH V6 0/2] mailbox: arm: introduce smc triggered mailbox

[PATCH V6 0/2] mailbox: arm: introduce smc triggered mailbox

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

V6:
Switch to per-channel a mbox controller
Drop arm,num-chans, transports, method
Add arm,hvc-mbox compatible
Fix smc/hvc args, drop client id and use correct type.

V5:
yaml fix
https://patchwork.kernel.org/cover/11117741/

V4:
yaml fix for num-chans in patch 1/2.
https://patchwork.kernel.org/cover/11116521/

V3:
Drop interrupt
Introduce transports for mem/reg usage
Add chan-id for mem usage
Convert to yaml format
https://patchwork.kernel.org/cover/11043541/
 
V2:
This is a modified version from Andre Przywara's patch series
https://lore.kernel.org/patchwork/cover/812997/.
The modification are mostly:
Introduce arm,num-chans
Introduce arm_smccc_mbox_cmd
txdone_poll and txdone_irq are both set to false
arm,func-ids are kept, but as an optional property.
Rewords SCPI to SCMI, because I am trying SCMI over SMC, not SCPI.
Introduce interrupts notification.

[1] is a draft implementation of i.MX8MM SCMI ATF implementation that
use smc as mailbox, power/clk is included, but only part of clk has been
implemented to work with hardware, power domain only supports get name
for now.

The traditional Linux mailbox mechanism uses some kind of dedicated hardware
IP to signal a condition to some other processing unit, typically a dedicated
management processor.
This mailbox feature is used for instance by the SCMI protocol to signal a
request for some action to be taken by the management processor.
However some SoCs does not have a dedicated management core to provide
those services. In order to service TEE and to avoid linux shutdown
power and clock that used by TEE, need let firmware to handle power
and clock, the firmware here is ARM Trusted Firmware that could also
run SCMI service.

The existing SCMI implementation uses a rather flexible shared memory
region to communicate commands and their parameters, it still requires a
mailbox to actually trigger the action.

This patch series provides a Linux mailbox compatible service which uses
smc calls to invoke firmware code, for instance taking care of SCMI requests.
The actual requests are still communicated using the standard SCMI way of
shared memory regions, but a dedicated mailbox hardware IP can be replaced via
this new driver.

This simple driver uses the architected SMC calling convention to trigger
firmware services, also allows for using "HVC" calls to call into hypervisors
or firmware layers running in the EL2 exception level.

Patch 1 contains the device tree binding documentation, patch 2 introduces
the actual mailbox driver.

Please note that this driver just provides a generic mailbox mechanism,
It could support synchronous TX/RX, or synchronous TX with asynchronous
RX. And while providing SCMI services was the reason for this exercise,
this driver is in no way bound to this use case, but can be used generically
where the OS wants to signal a mailbox condition to firmware or a
hypervisor.
Also the driver is in no way meant to replace any existing firmware
interface, but actually to complement existing interfaces.

[1] https://github.com/MrVan/arm-trusted-firmware/tree/scmi




Peng Fan (2):
  dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
  mailbox: introduce ARM SMC based mailbox

 .../devicetree/bindings/mailbox/arm-smc.yaml       |  96 ++++++++++++
 drivers/mailbox/Kconfig                            |   7 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/arm-smc-mailbox.c                  | 167 +++++++++++++++++++++
 4 files changed, 272 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c

-- 
2.16.4

[PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
actions in software layers running in the EL2 or EL3 exception levels.
The term "ARM" here relates to the SMC instruction as part of the ARM
instruction set, not as a standard endorsed by ARM Ltd.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../devicetree/bindings/mailbox/arm-smc.yaml       | 96 ++++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
new file mode 100644
index 000000000000..bf01bec035fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM SMC Mailbox Interface
+
+maintainers:
+  - Peng Fan <peng.fan@nxp.com>
+
+description: |
+  This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
+  call) instruction to trigger a mailbox-connected activity in firmware,
+  executing on the very same core as the caller. The value of r0/w0/x0
+  the firmware returns after the smc call is delivered as a received
+  message to the mailbox framework, so synchronous communication can be
+  established. The exact meaning of the action the mailbox triggers as
+  well as the return value is defined by their users and is not subject
+  to this binding.
+
+  One use case of this mailbox is the SCMI interface, which uses shared
+  memory to transfer commands and parameters, and a mailbox to trigger a
+  function call. This allows SoCs without a separate management processor
+  (or when such a processor is not available or used) to use this
+  standardized interface anyway.
+
+  This binding describes no hardware, but establishes a firmware interface.
+  Upon receiving an SMC using one of the described SMC function identifiers,
+  the firmware is expected to trigger some mailbox connected functionality.
+  The communication follows the ARM SMC calling convention.
+  Firmware expects an SMC function identifier in r0 or w0. The supported
+  identifiers are passed from consumers, or listed in the the arm,func-ids
+  properties as described below. The firmware can return one value in
+  the first SMC result register, it is expected to be an error value,
+  which shall be propagated to the mailbox client.
+
+  Any core which supports the SMC or HVC instruction can be used, as long
+  as a firmware component running in EL3 or EL2 is handling these calls.
+
+properties:
+  compatible:
+    oneOf:
+      - description:
+          For implementations using ARM SMC instruction.
+        const: arm,smc-mbox
+
+      - description:
+          For implementations using ARM HVC instruction.
+        const: arm,hvc-mbox
+
+  "#mbox-cells":
+    const: 1
+
+  arm,func-id:
+    description: |
+      An 32-bit value specifying the function ID used by the mailbox.
+      The function ID follow the ARM SMC calling convention standard [1].
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - "#mbox-cells"
+
+examples:
+  - |
+    sram@93f000 {
+      compatible = "mmio-sram";
+      reg = <0x0 0x93f000 0x0 0x1000>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      ranges = <0x0 0x93f000 0x1000>;
+
+      cpu_scp_lpri: scp-shmem@0 {
+        compatible = "arm,scmi-shmem";
+        reg = <0x0 0x200>;
+      };
+    };
+
+    smc_tx_mbox: tx_mbox {
+      #mbox-cells = <1>;
+      compatible = "arm,smc-mbox";
+      /* optional */
+      arm,func-id = <0xc20000fe>;
+    };
+
+    firmware {
+      scmi {
+        compatible = "arm,scmi";
+        mboxes = <&smc_tx_mbox 0>;
+        mbox-names = "tx";
+        shmem = <&cpu_scp_lpri>;
+      };
+    };
+
+...
-- 
2.16.4

[PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Peng Fan]

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/

Cc: 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 | 167 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 176 insertions(+)
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c

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..c84aef39c8d9
--- /dev/null
+++ b/drivers/mailbox/arm-smc-mailbox.c
@@ -0,0 +1,167 @@
+// 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/interrupt.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+struct arm_smc_chan_data {
+	unsigned int function_id;
+};
+
+struct arm_smccc_mbox_cmd {
+	unsigned int function_id;
+	union {
+		unsigned int args_smccc32[6];
+		unsigned long args_smccc64[6];
+	};
+};
+
+typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
+				    unsigned long, unsigned long,
+				    unsigned long, unsigned long,
+				    unsigned long);
+static 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;
+	u32 function_id;
+
+	function_id = chan_data->function_id;
+	if (!function_id)
+		function_id = cmd->function_id;
+
+	if (function_id & BIT(30)) {
+		ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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 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;
+	u32 function_id = 0;
+
+	if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
+		invoke_smc_mbox_fn = __invoke_fn_smc;
+	else
+		invoke_smc_mbox_fn = __invoke_fn_hvc;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	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;
+
+	of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
+	chan_data->function_id = function_id;
+
+	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");
-- 
2.16.4

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Andre Przywara]

On Mon, 16 Sep 2019 09:44:37 +0000
Peng Fan <peng.fan@nxp.com> wrote:

Hi,

> From: Peng Fan <peng.fan@nxp.com>
> 
> The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> actions in software layers running in the EL2 or EL3 exception levels.
> The term "ARM" here relates to the SMC instruction as part of the ARM
> instruction set, not as a standard endorsed by ARM Ltd.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/mailbox/arm-smc.yaml       | 96 ++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> new file mode 100644
> index 000000000000..bf01bec035fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMC Mailbox Interface
> +
> +maintainers:
> +  - Peng Fan <peng.fan@nxp.com>
> +
> +description: |
> +  This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor

I think "or" instead of "and" is less confusing.

> +  call) instruction to trigger a mailbox-connected activity in firmware,
> +  executing on the very same core as the caller. The value of r0/w0/x0
> +  the firmware returns after the smc call is delivered as a received
> +  message to the mailbox framework, so synchronous communication can be
> +  established. The exact meaning of the action the mailbox triggers as
> +  well as the return value is defined by their users and is not subject
> +  to this binding.
> +
> +  One use case of this mailbox is the SCMI interface, which uses shared

     One example use case of this mailbox ...
(to make it more obvious that it's not restricted to this)

> +  memory to transfer commands and parameters, and a mailbox to trigger a
> +  function call. This allows SoCs without a separate management processor
> +  (or when such a processor is not available or used) to use this
> +  standardized interface anyway.
> +
> +  This binding describes no hardware, but establishes a firmware interface.
> +  Upon receiving an SMC using one of the described SMC function identifiers,

                             ... the described SMC function identifier,

> +  the firmware is expected to trigger some mailbox connected functionality.
> +  The communication follows the ARM SMC calling convention.
> +  Firmware expects an SMC function identifier in r0 or w0. The supported
> +  identifiers are passed from consumers,

     identifier

"passed from consumers": How? Where?
But I want to repeat: We should not allow this. This is a binding for a mailbox controller driver, not a generic firmware backdoor.
We should be as strict as possible to avoid any security issues.
The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).

What would be the use case of this functionality?

> or listed in the the arm,func-ids

                       arm,func-id

> +  properties as described below. The firmware can return one value in

     property

> +  the first SMC result register, it is expected to be an error value,
> +  which shall be propagated to the mailbox client.
> +
> +  Any core which supports the SMC or HVC instruction can be used, as long
> +  as a firmware component running in EL3 or EL2 is handling these calls.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description:
> +          For implementations using ARM SMC instruction.
> +        const: arm,smc-mbox
> +
> +      - description:
> +          For implementations using ARM HVC instruction.
> +        const: arm,hvc-mbox

I am not particularly happy with this, but well ...

> +
> +  "#mbox-cells":
> +    const: 1

Why is this "1"? What is this number used for? It used to be the channel ID, but since you are describing a single channel controller only, this should be 0 now.

> +
> +  arm,func-id:
> +    description: |
> +      An 32-bit value specifying the function ID used by the mailbox.

         A single 32-bit value ...

> +      The function ID follow the ARM SMC calling convention standard [1].

                         follows

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - "#mbox-cells"
> +
> +examples:
> +  - |
> +    sram@93f000 {
> +      compatible = "mmio-sram";
> +      reg = <0x0 0x93f000 0x0 0x1000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      ranges = <0x0 0x93f000 0x1000>;
> +
> +      cpu_scp_lpri: scp-shmem@0 {
> +        compatible = "arm,scmi-shmem";
> +        reg = <0x0 0x200>;
> +      };
> +    };
> +
> +    smc_tx_mbox: tx_mbox {
> +      #mbox-cells = <1>;

As mentioned above, should be 0.

> +      compatible = "arm,smc-mbox";
> +      /* optional */

First: having "optional" in a specific example is not helpful, just confusing.
Second: It is actually *not* optional in this case, as there is no other way of propagating the function ID. The SCMI driver as the mailbox client has certainly no clue about this.
I think I said this previously: Relying on the mailbox client to pass the function ID sounds broken, as this is a property of the mailbox controller driver. The mailbox client does not care about this mailbox communication detail, it just wants to trigger the mailbox.

> +      arm,func-id = <0xc20000fe>;
> +    };
> +
> +    firmware {
> +      scmi {
> +        compatible = "arm,scmi";
> +        mboxes = <&smc_tx_mbox 0>;

... and here just <&smc_tx_mbox>; would suffice.

> +        mbox-names = "tx";
> +        shmem = <&cpu_scp_lpri>;
> +      };
> +    };
> +
> +...

Cheers,
Andre.

Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Andre Przywara]

On Mon, 16 Sep 2019 09:44:41 +0000
Peng Fan <peng.fan@nxp.com> wrote:

Hi,

looks quite good now, some smaller comments below.
I think the only thing left is the "function ID passed by the client" topic.

Have you tried using this interface using hvc, for instance in KVM or Xen? For instance to provide access to a clock for a passed-through platform device?
Another use case that pops up from time to time is GPIO for guests. This sounds like a use case for using the register interface, also we could use the hvc return value.

> 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/
> 
> Cc: 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 | 167 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 176 insertions(+)
>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
> 
> 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..c84aef39c8d9
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,167 @@
> +// 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/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +struct arm_smc_chan_data {
> +	unsigned int function_id;
> +};
> +
> +struct arm_smccc_mbox_cmd {
> +	unsigned int function_id;
> +	union {
> +		unsigned int args_smccc32[6];
> +		unsigned long args_smccc64[6];

Shouldn't this be u32 and u64 here, as the data type has this explicit length in the SMCCC?

> +	};
> +};

If this is the data structure that this mailbox controller uses, I would expect this to be documented somewhere, or even exported.

And again, I don't like the idea of having the function ID in here.

> +
> +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> +				    unsigned long, unsigned long,
> +				    unsigned long, unsigned long,
> +				    unsigned long);
> +static 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;
> +	u32 function_id;
> +
> +	function_id = chan_data->function_id;
> +	if (!function_id)
> +		function_id = cmd->function_id;
> +
> +	if (function_id & BIT(30)) {

	if (ARM_SMCCC_IS_64(function_id)) {

> +		ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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 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;
> +	u32 function_id = 0;
> +
> +	if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> +		invoke_smc_mbox_fn = __invoke_fn_smc;
> +	else
> +		invoke_smc_mbox_fn = __invoke_fn_hvc;
> +
> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (!mbox)
> +		return -ENOMEM;
> +
> +	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;
> +
> +	of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
> +	chan_data->function_id = function_id;
> +
> +	mbox->chans->con_priv = chan_data;
> +
> +	mbox->txdone_poll = false;
> +	mbox->txdone_irq = false;

Don't we need to provide something to confirm reception to the client? In our case we can set this as soon as the smc/hvc returns.

Cheers,
Andre.

> +	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");

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Jassi Brar]

On Tue, Sep 17, 2019 at 12:31 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Mon, 16 Sep 2019 09:44:37 +0000
> Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi,
>
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> > actions in software layers running in the EL2 or EL3 exception levels.
> > The term "ARM" here relates to the SMC instruction as part of the ARM
> > instruction set, not as a standard endorsed by ARM Ltd.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../devicetree/bindings/mailbox/arm-smc.yaml       | 96 ++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > new file mode 100644
> > index 000000000000..bf01bec035fc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,96 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM SMC Mailbox Interface
> > +
> > +maintainers:
> > +  - Peng Fan <peng.fan@nxp.com>
> > +
> > +description: |
> > +  This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
>
> I think "or" instead of "and" is less confusing.
>
> > +  call) instruction to trigger a mailbox-connected activity in firmware,
> > +  executing on the very same core as the caller. The value of r0/w0/x0
> > +  the firmware returns after the smc call is delivered as a received
> > +  message to the mailbox framework, so synchronous communication can be
> > +  established. The exact meaning of the action the mailbox triggers as
> > +  well as the return value is defined by their users and is not subject
> > +  to this binding.
> > +
> > +  One use case of this mailbox is the SCMI interface, which uses shared
>
>      One example use case of this mailbox ...
> (to make it more obvious that it's not restricted to this)
>
> > +  memory to transfer commands and parameters, and a mailbox to trigger a
> > +  function call. This allows SoCs without a separate management processor
> > +  (or when such a processor is not available or used) to use this
> > +  standardized interface anyway.
> > +
> > +  This binding describes no hardware, but establishes a firmware interface.
> > +  Upon receiving an SMC using one of the described SMC function identifiers,
>
>                              ... the described SMC function identifier,
>
> > +  the firmware is expected to trigger some mailbox connected functionality.
> > +  The communication follows the ARM SMC calling convention.
> > +  Firmware expects an SMC function identifier in r0 or w0. The supported
> > +  identifiers are passed from consumers,
>
>      identifier
>
> "passed from consumers": How? Where?
> But I want to repeat: We should not allow this.
> This is a binding for a mailbox controller driver, not a generic firmware backdoor.
>
Exactly. The mailbox controller here is the  SMC/HVC instruction,
which needs 9 arguments to work. The fact that the fist argument is
always going to be same on a platform is just the way we use this
instruction.

> We should be as strict as possible to avoid any security issues.
>
Any example of such a security issue?

> The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
>
> What would be the use case of this functionality?
>
At least for flexibility and consistency.

> > or listed in the the arm,func-ids
>
>                        arm,func-id
>
> > +  properties as described below. The firmware can return one value in
>
>      property
>
> > +  the first SMC result register, it is expected to be an error value,
> > +  which shall be propagated to the mailbox client.
> > +
> > +  Any core which supports the SMC or HVC instruction can be used, as long
> > +  as a firmware component running in EL3 or EL2 is handling these calls.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - description:
> > +          For implementations using ARM SMC instruction.
> > +        const: arm,smc-mbox
> > +
> > +      - description:
> > +          For implementations using ARM HVC instruction.
> > +        const: arm,hvc-mbox
>
> I am not particularly happy with this, but well ...
>
> > +
> > +  "#mbox-cells":
> > +    const: 1
>
> Why is this "1"? What is this number used for? It used to be the channel ID, but since you are describing a single channel controller only, this should be 0 now.
>
Yes. I overlooked it and actually queued the patch for pull request.
But I think the bindings should not carry a 'fix' patch later. Also I
realise this revision of binding hasn't been reviewed by Rob. Maybe I
should drop the patch for now.

> > +
> > +  arm,func-id:
> > +    description: |
> > +      An 32-bit value specifying the function ID used by the mailbox.
>
>          A single 32-bit value ...
>
> > +      The function ID follow the ARM SMC calling convention standard [1].
>
>                          follows
>
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - "#mbox-cells"
> > +
> > +examples:
> > +  - |
> > +    sram@93f000 {
> > +      compatible = "mmio-sram";
> > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +      ranges = <0x0 0x93f000 0x1000>;
> > +
> > +      cpu_scp_lpri: scp-shmem@0 {
> > +        compatible = "arm,scmi-shmem";
> > +        reg = <0x0 0x200>;
> > +      };
> > +    };
> > +
> > +    smc_tx_mbox: tx_mbox {
> > +      #mbox-cells = <1>;
>
> As mentioned above, should be 0.
>
> > +      compatible = "arm,smc-mbox";
> > +      /* optional */
>
> First: having "optional" in a specific example is not helpful, just confusing.
> Second: It is actually *not* optional in this case, as there is no other way of propagating the function ID. The SCMI driver as the mailbox client has certainly no clue about this.
> I think I said this previously: Relying on the mailbox client to pass the function ID sounds broken, as this is a property of the mailbox controller driver. The mailbox client does not care about this mailbox communication detail, it just wants to trigger the mailbox.
>
Again, the mailbox controller here is the SMC/HVC _instruction_, which
doesn't care what value the first argument carry.

Cheers!

RE: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Peng Fan]

Hi Jassi,

> Subject: Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the
> ARM SMC/HVC mailbox
> 
> On Tue, Sep 17, 2019 at 12:31 PM Andre Przywara
> <andre.przywara@arm.com> wrote:
> >
> > On Mon, 16 Sep 2019 09:44:37 +0000
> > Peng Fan <peng.fan@nxp.com> wrote:
> >
> > Hi,
> >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The ARM SMC/HVC mailbox binding describes a firmware interface to
> > > trigger actions in software layers running in the EL2 or EL3 exception
> levels.
> > > The term "ARM" here relates to the SMC instruction as part of the
> > > ARM instruction set, not as a standard endorsed by ARM Ltd.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  .../devicetree/bindings/mailbox/arm-smc.yaml       | 96
> ++++++++++++++++++++++
> > >  1 file changed, 96 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > new file mode 100644
> > > index 000000000000..bf01bec035fc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > @@ -0,0 +1,96 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > >
> +vicetree.org%2Fschemas%2Fmailbox%2Farm-smc.yaml%23&amp;data=02%
> 7C01
> > >
> +%7Cpeng.fan%40nxp.com%7Cf8065d24dd474238baf008d73bf8dc7a%7C686
> ea1d3
> > >
> +bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637043812342903260&amp;sd
> ata=vC3
> > >
> +S8hvYDxDhNbIQoC44hpO5bw1yYZdBwu%2B%2Fp8mV0hI%3D&amp;reserv
> ed=0
> > > +$schema:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > >
> +vicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=02%7C01%7C
> peng.
> > >
> +fan%40nxp.com%7Cf8065d24dd474238baf008d73bf8dc7a%7C686ea1d3bc2
> b4c6f
> > >
> +a92cd99c5c301635%7C0%7C1%7C637043812342903260&amp;sdata=IDHd
> vf1Mgw1
> > > +BR%2Bo4XJ%2BjQS%2Bx1pSBzADnW44B2hZLzKw%3D&amp;reserved=0
> > > +
> > > +title: ARM SMC Mailbox Interface
> > > +
> > > +maintainers:
> > > +  - Peng Fan <peng.fan@nxp.com>
> > > +
> > > +description: |
> > > +  This mailbox uses the ARM smc (secure monitor call) and hvc
> > > +(hypervisor
> >
> > I think "or" instead of "and" is less confusing.
> >
> > > +  call) instruction to trigger a mailbox-connected activity in
> > > + firmware,  executing on the very same core as the caller. The
> > > + value of r0/w0/x0  the firmware returns after the smc call is
> > > + delivered as a received  message to the mailbox framework, so
> > > + synchronous communication can be  established. The exact meaning
> > > + of the action the mailbox triggers as  well as the return value is
> > > + defined by their users and is not subject  to this binding.
> > > +
> > > +  One use case of this mailbox is the SCMI interface, which uses
> > > + shared
> >
> >      One example use case of this mailbox ...
> > (to make it more obvious that it's not restricted to this)
> >
> > > +  memory to transfer commands and parameters, and a mailbox to
> > > + trigger a  function call. This allows SoCs without a separate
> > > + management processor  (or when such a processor is not available
> > > + or used) to use this  standardized interface anyway.
> > > +
> > > +  This binding describes no hardware, but establishes a firmware
> interface.
> > > +  Upon receiving an SMC using one of the described SMC function
> > > + identifiers,
> >
> >                              ... the described SMC function
> > identifier,
> >
> > > +  the firmware is expected to trigger some mailbox connected
> functionality.
> > > +  The communication follows the ARM SMC calling convention.
> > > +  Firmware expects an SMC function identifier in r0 or w0. The
> > > + supported  identifiers are passed from consumers,
> >
> >      identifier
> >
> > "passed from consumers": How? Where?
> > But I want to repeat: We should not allow this.
> > This is a binding for a mailbox controller driver, not a generic firmware
> backdoor.
> >
> Exactly. The mailbox controller here is the  SMC/HVC instruction, which
> needs 9 arguments to work. The fact that the fist argument is always going to
> be same on a platform is just the way we use this instruction.
> 
> > We should be as strict as possible to avoid any security issues.
> >
> Any example of such a security issue?
> 
> > The firmware certainly knows the function ID it implements. The firmware
> controls the DT. So it is straight-forward to put the ID into the DT. The
> firmware could even do this at boot time, dynamically, before passing on the
> DT to the non-secure world (bootloader or kernel).
> >
> > What would be the use case of this functionality?
> >
> At least for flexibility and consistency.
> 
> > > or listed in the the arm,func-ids
> >
> >                        arm,func-id
> >
> > > +  properties as described below. The firmware can return one value
> > > + in
> >
> >      property
> >
> > > +  the first SMC result register, it is expected to be an error
> > > + value,  which shall be propagated to the mailbox client.
> > > +
> > > +  Any core which supports the SMC or HVC instruction can be used,
> > > + as long  as a firmware component running in EL3 or EL2 is handling
> these calls.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - description:
> > > +          For implementations using ARM SMC instruction.
> > > +        const: arm,smc-mbox
> > > +
> > > +      - description:
> > > +          For implementations using ARM HVC instruction.
> > > +        const: arm,hvc-mbox
> >
> > I am not particularly happy with this, but well ...
> >
> > > +
> > > +  "#mbox-cells":
> > > +    const: 1
> >
> > Why is this "1"? What is this number used for? It used to be the channel ID,
> but since you are describing a single channel controller only, this should be 0
> now.
> >
> Yes. I overlooked it and actually queued the patch for pull request.

In Documentation/devicetree/bindings/mailbox/mailbox.txt
#mbox-cells: Must be at least 1.

So I use 1 here, 0 not work. Because of_mbox_index_xlate expect at least 1 here.
So I need modify Documentation/devicetree/bindings/mailbox/mailbox.txt
and add xlate for smc mailbox?

Thanks,
Peng.

> But I think the bindings should not carry a 'fix' patch later. Also I realise this
> revision of binding hasn't been reviewed by Rob. Maybe I should drop the
> patch for now.
> 
> > > +
> > > +  arm,func-id:
> > > +    description: |
> > > +      An 32-bit value specifying the function ID used by the mailbox.
> >
> >          A single 32-bit value ...
> >
> > > +      The function ID follow the ARM SMC calling convention standard
> [1].
> >
> >                          follows
> >
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +required:
> > > +  - compatible
> > > +  - "#mbox-cells"
> > > +
> > > +examples:
> > > +  - |
> > > +    sram@93f000 {
> > > +      compatible = "mmio-sram";
> > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > +      #address-cells = <1>;
> > > +      #size-cells = <1>;
> > > +      ranges = <0x0 0x93f000 0x1000>;
> > > +
> > > +      cpu_scp_lpri: scp-shmem@0 {
> > > +        compatible = "arm,scmi-shmem";
> > > +        reg = <0x0 0x200>;
> > > +      };
> > > +    };
> > > +
> > > +    smc_tx_mbox: tx_mbox {
> > > +      #mbox-cells = <1>;
> >
> > As mentioned above, should be 0.
> >
> > > +      compatible = "arm,smc-mbox";
> > > +      /* optional */
> >
> > First: having "optional" in a specific example is not helpful, just confusing.
> > Second: It is actually *not* optional in this case, as there is no other way of
> propagating the function ID. The SCMI driver as the mailbox client has
> certainly no clue about this.
> > I think I said this previously: Relying on the mailbox client to pass the
> function ID sounds broken, as this is a property of the mailbox controller driver.
> The mailbox client does not care about this mailbox communication detail, it
> just wants to trigger the mailbox.
> >
> Again, the mailbox controller here is the SMC/HVC _instruction_, which
> doesn't care what value the first argument carry.
> 
> Cheers!

RE: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Peng Fan]

Hi Andre,

> Subject: Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the
> ARM SMC/HVC mailbox
> 
> On Mon, 16 Sep 2019 09:44:37 +0000
> Peng Fan <peng.fan@nxp.com> wrote:
> 
> Hi,
> 
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The ARM SMC/HVC mailbox binding describes a firmware interface to
> > trigger actions in software layers running in the EL2 or EL3 exception levels.
> > The term "ARM" here relates to the SMC instruction as part of the ARM
> > instruction set, not as a standard endorsed by ARM Ltd.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../devicetree/bindings/mailbox/arm-smc.yaml       | 96
> ++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > new file mode 100644
> > index 000000000000..bf01bec035fc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,96 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fmailbox%2Farm-smc.yaml%23&amp;data=02%7
> C01%7Cp
> >
> +eng.fan%40nxp.com%7Cff378bc3d622436c39ba08d73b94dfcc%7C686ea1d
> 3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C1%7C637043382928045369&amp;sdata=rnx
> KdDGjPPd
> > +8VBI5WmgnZ3jxIjL2hcRYzbljfFxDkA0%3D&amp;reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=02%7C01%7Cpe
> ng.fan%
> >
> +40nxp.com%7Cff378bc3d622436c39ba08d73b94dfcc%7C686ea1d3bc2b4c6
> fa92cd9
> >
> +9c5c301635%7C0%7C1%7C637043382928045369&amp;sdata=R02nWzpp9
> %2BrDYG9tA
> > +ot4pdWb8tGGHet1MOjrD0dEjwA%3D&amp;reserved=0
> > +
> > +title: ARM SMC Mailbox Interface
> > +
> > +maintainers:
> > +  - Peng Fan <peng.fan@nxp.com>
> > +
> > +description: |
> > +  This mailbox uses the ARM smc (secure monitor call) and hvc
> > +(hypervisor
> 
> I think "or" instead of "and" is less confusing.

ok

> 
> > +  call) instruction to trigger a mailbox-connected activity in
> > + firmware,  executing on the very same core as the caller. The value
> > + of r0/w0/x0  the firmware returns after the smc call is delivered as
> > + a received  message to the mailbox framework, so synchronous
> > + communication can be  established. The exact meaning of the action
> > + the mailbox triggers as  well as the return value is defined by
> > + their users and is not subject  to this binding.
> > +
> > +  One use case of this mailbox is the SCMI interface, which uses
> > + shared
> 
>      One example use case of this mailbox ...
> (to make it more obvious that it's not restricted to this)

ok

> 
> > +  memory to transfer commands and parameters, and a mailbox to
> > + trigger a  function call. This allows SoCs without a separate
> > + management processor  (or when such a processor is not available or
> > + used) to use this  standardized interface anyway.
> > +
> > +  This binding describes no hardware, but establishes a firmware
> interface.
> > +  Upon receiving an SMC using one of the described SMC function
> > + identifiers,
> 
>                              ... the described SMC function identifier,

ok

> 
> > +  the firmware is expected to trigger some mailbox connected
> functionality.
> > +  The communication follows the ARM SMC calling convention.
> > +  Firmware expects an SMC function identifier in r0 or w0. The
> > + supported  identifiers are passed from consumers,
> 
>      identifier

ok

> 
> "passed from consumers": How? Where?
> But I want to repeat: We should not allow this. This is a binding for a mailbox
> controller driver, not a generic firmware backdoor.

As Jassi suggested the function identifier as an optional for mailbox driver.
The driver should support function id passed from consumers.
Currently there is no users for such case that passed from consumers,
so I have no idea how.

> We should be as strict as possible to avoid any security issues.
> The firmware certainly knows the function ID it implements. The firmware
> controls the DT. So it is straight-forward to put the ID into the DT. The
> firmware could even do this at boot time, dynamically, before passing on the
> DT to the non-secure world (bootloader or kernel).
> 
> What would be the use case of this functionality?
> 
> > or listed in the the arm,func-ids
> 
>                        arm,func-id

ok
> 
> > +  properties as described below. The firmware can return one value in
> 
>      property
ok
> 
> > +  the first SMC result register, it is expected to be an error value,
> > + which shall be propagated to the mailbox client.
> > +
> > +  Any core which supports the SMC or HVC instruction can be used, as
> > + long  as a firmware component running in EL3 or EL2 is handling these
> calls.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - description:
> > +          For implementations using ARM SMC instruction.
> > +        const: arm,smc-mbox
> > +
> > +      - description:
> > +          For implementations using ARM HVC instruction.
> > +        const: arm,hvc-mbox
> 
> I am not particularly happy with this, but well ...
> 
> > +
> > +  "#mbox-cells":
> > +    const: 1
> 
> Why is this "1"? What is this number used for? It used to be the channel ID,
> but since you are describing a single channel controller only, this should be 0
> now.

Mailbox bindings requires it at least 1, as replied to Jassi in the other mail.

> 
> > +
> > +  arm,func-id:
> > +    description: |
> > +      An 32-bit value specifying the function ID used by the mailbox.
> 
>          A single 32-bit value ...
> 
> > +      The function ID follow the ARM SMC calling convention standard
> [1].
> 
>                          follows
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - "#mbox-cells"
> > +
> > +examples:
> > +  - |
> > +    sram@93f000 {
> > +      compatible = "mmio-sram";
> > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +      ranges = <0x0 0x93f000 0x1000>;
> > +
> > +      cpu_scp_lpri: scp-shmem@0 {
> > +        compatible = "arm,scmi-shmem";
> > +        reg = <0x0 0x200>;
> > +      };
> > +    };
> > +
> > +    smc_tx_mbox: tx_mbox {
> > +      #mbox-cells = <1>;
> 
> As mentioned above, should be 0.
> 
> > +      compatible = "arm,smc-mbox";
> > +      /* optional */
> 
> First: having "optional" in a specific example is not helpful, just confusing.
> Second: It is actually *not* optional in this case, as there is no other way of
> propagating the function ID. The SCMI driver as the mailbox client has
> certainly no clue about this.

I'll drop "/*optinal*/" since it is required in the example.

> I think I said this previously: Relying on the mailbox client to pass the function
> ID sounds broken, as this is a property of the mailbox controller driver. The
> mailbox client does not care about this mailbox communication detail, it just
> wants to trigger the mailbox.
> 
> > +      arm,func-id = <0xc20000fe>;
> > +    };
> > +
> > +    firmware {
> > +      scmi {
> > +        compatible = "arm,scmi";
> > +        mboxes = <&smc_tx_mbox 0>;
> 
> ... and here just <&smc_tx_mbox>; would suffice.

Mailbox requires mbox-cells at least 1, it must have one arg.
Otherwise of_mbox_index_xlate not work.

Thanks,
Peng.

> 
> > +        mbox-names = "tx";
> > +        shmem = <&cpu_scp_lpri>;
> > +      };
> > +    };
> > +
> > +...
> 
> Cheers,
> Andre.

RE: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Peng Fan]

Hi Andre,

> Subject: Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, 16 Sep 2019 09:44:41 +0000
> Peng Fan <peng.fan@nxp.com> wrote:
> 
> Hi,
> 
> looks quite good now, some smaller comments below.
> I think the only thing left is the "function ID passed by the client" topic.
> 
> Have you tried using this interface using hvc, for instance in KVM or Xen? For

No. I do not have that implementation in hypervisor.

> instance to provide access to a clock for a passed-through platform device?
> Another use case that pops up from time to time is GPIO for guests. This
> sounds like a use case for using the register interface, also we could use the
> hvc return value.
> 
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
> Cpeng.fa
> >
> n%40nxp.com%7C58a1ed4078264d14958f08d73b95ed7e%7C686ea1d3bc2b
> 4c6fa92cd
> >
> 99c5c301635%7C0%7C1%7C637043387484474825&amp;sdata=Cp1zlhlpQbg
> BsWu9ZDV
> > RKkXmd6kvUR%2BtPO7EPR7YLpA%3D&amp;reserved=0
> >
> > Cc: 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 | 167
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 176 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
> >
> > 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..c84aef39c8d9
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,167 @@
> > +// 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/interrupt.h>
> > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +struct arm_smc_chan_data {
> > +	unsigned int function_id;
> > +};
> > +
> > +struct arm_smccc_mbox_cmd {
> > +	unsigned int function_id;
> > +	union {
> > +		unsigned int args_smccc32[6];
> > +		unsigned long args_smccc64[6];
> 
> Shouldn't this be u32 and u64 here, as the data type has this explicit length in
> the SMCCC?

ok

> 
> > +	};
> > +};
> 
> If this is the data structure that this mailbox controller uses, I would expect
> this to be documented somewhere, or even exported.

Export this structure in include/linux/mailbox/smc-mailbox.h?

> 
> And again, I don't like the idea of having the function ID in here.

You mean function_id in arm_smccc_mbox_cmd is not good?

> 
> > +
> > +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> > +				    unsigned long, unsigned long,
> > +				    unsigned long, unsigned long,
> > +				    unsigned long);
> > +static 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;
> > +	u32 function_id;
> > +
> > +	function_id = chan_data->function_id;
> > +	if (!function_id)
> > +		function_id = cmd->function_id;
> > +
> > +	if (function_id & BIT(30)) {
> 
> 	if (ARM_SMCCC_IS_64(function_id)) {

ok

> 
> > +		ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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 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;
> > +	u32 function_id = 0;
> > +
> > +	if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> > +		invoke_smc_mbox_fn = __invoke_fn_smc;
> > +	else
> > +		invoke_smc_mbox_fn = __invoke_fn_hvc;
> > +
> > +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > +	if (!mbox)
> > +		return -ENOMEM;
> > +
> > +	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;
> > +
> > +	of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
> > +	chan_data->function_id = function_id;
> > +
> > +	mbox->chans->con_priv = chan_data;
> > +
> > +	mbox->txdone_poll = false;
> > +	mbox->txdone_irq = false;
> 
> Don't we need to provide something to confirm reception to the client? In our
> case we can set this as soon as the smc/hvc returns.

As smc/hvc returns, it means the transfer is over and receive is done.

Thanks,
Peng.

> 
> Cheers,
> Andre.
> 
> > +	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");

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Andre Przywara]

On Wed, 18 Sep 2019 00:27:00 -0500
Jassi Brar <jassisinghbrar@gmail.com> wrote:

Hi,

> On Tue, Sep 17, 2019 at 12:31 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Mon, 16 Sep 2019 09:44:37 +0000
> > Peng Fan <peng.fan@nxp.com> wrote:
> >
> > Hi,
> >  
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> > > actions in software layers running in the EL2 or EL3 exception levels.
> > > The term "ARM" here relates to the SMC instruction as part of the ARM
> > > instruction set, not as a standard endorsed by ARM Ltd.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  .../devicetree/bindings/mailbox/arm-smc.yaml       | 96 ++++++++++++++++++++++
> > >  1 file changed, 96 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > new file mode 100644
> > > index 000000000000..bf01bec035fc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > > @@ -0,0 +1,96 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ARM SMC Mailbox Interface
> > > +
> > > +maintainers:
> > > +  - Peng Fan <peng.fan@nxp.com>
> > > +
> > > +description: |
> > > +  This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor  
> >
> > I think "or" instead of "and" is less confusing.
> >  
> > > +  call) instruction to trigger a mailbox-connected activity in firmware,
> > > +  executing on the very same core as the caller. The value of r0/w0/x0
> > > +  the firmware returns after the smc call is delivered as a received
> > > +  message to the mailbox framework, so synchronous communication can be
> > > +  established. The exact meaning of the action the mailbox triggers as
> > > +  well as the return value is defined by their users and is not subject
> > > +  to this binding.
> > > +
> > > +  One use case of this mailbox is the SCMI interface, which uses shared  
> >
> >      One example use case of this mailbox ...
> > (to make it more obvious that it's not restricted to this)
> >  
> > > +  memory to transfer commands and parameters, and a mailbox to trigger a
> > > +  function call. This allows SoCs without a separate management processor
> > > +  (or when such a processor is not available or used) to use this
> > > +  standardized interface anyway.
> > > +
> > > +  This binding describes no hardware, but establishes a firmware interface.
> > > +  Upon receiving an SMC using one of the described SMC function identifiers,  
> >
> >                              ... the described SMC function identifier,
> >  
> > > +  the firmware is expected to trigger some mailbox connected functionality.
> > > +  The communication follows the ARM SMC calling convention.
> > > +  Firmware expects an SMC function identifier in r0 or w0. The supported
> > > +  identifiers are passed from consumers,  
> >
> >      identifier
> >
> > "passed from consumers": How? Where?
> > But I want to repeat: We should not allow this.
> > This is a binding for a mailbox controller driver, not a generic firmware backdoor.
> >  
> Exactly. The mailbox controller here is the  SMC/HVC instruction,

No, the mailbox controller is an *SMCCC compliant* smc/hvc call, targeting a very specific function ID. SMC calls are used for PSCI already, for instance, and you don't want to mess with that. Also some platforms define a vendor specific smc interface, again using a well constructed function ID complying to SMCCC.
So we definitely need to stay within SMCCC for this kind of generic interface, *and* to let firmware specify the function ID via the DT, to not clash with any other function ID.

> which needs 9 arguments to work. The fact that the fist argument is
> always going to be same on a platform is just the way we use this
> instruction.
> 
> > We should be as strict as possible to avoid any security issues.
> >  
> Any example of such a security issue?

Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.

Do you have any example of a use case where the mailbox client needs to provide the function ID?

> > The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
> >
> > What would be the use case of this functionality?
> >  
> At least for flexibility and consistency.

I appreciate the flexibility idea, but when creating an interface, especially a generic one to any kind of firmware, you should be as strict as possible, to avoid clashes in the future.
 
> > > or listed in the the arm,func-ids  
> >
> >                        arm,func-id
> >  
> > > +  properties as described below. The firmware can return one value in  
> >
> >      property
> >  
> > > +  the first SMC result register, it is expected to be an error value,
> > > +  which shall be propagated to the mailbox client.
> > > +
> > > +  Any core which supports the SMC or HVC instruction can be used, as long
> > > +  as a firmware component running in EL3 or EL2 is handling these calls.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - description:
> > > +          For implementations using ARM SMC instruction.
> > > +        const: arm,smc-mbox
> > > +
> > > +      - description:
> > > +          For implementations using ARM HVC instruction.
> > > +        const: arm,hvc-mbox  
> >
> > I am not particularly happy with this, but well ...
> >  
> > > +
> > > +  "#mbox-cells":
> > > +    const: 1  
> >
> > Why is this "1"? What is this number used for? It used to be the channel ID, but since you are describing a single channel controller only, this should be 0 now.
> >  
> Yes. I overlooked it and actually queued the patch for pull request.
> But I think the bindings should not carry a 'fix' patch later. Also I
> realise this revision of binding hasn't been reviewed by Rob. Maybe I
> should drop the patch for now.

Yes, please do. I would like to make sure that the binding is correct, as it serves as a specification for people implementing both firmware services and other drivers (like *BSD).

> > > +
> > > +  arm,func-id:
> > > +    description: |
> > > +      An 32-bit value specifying the function ID used by the mailbox.  
> >
> >          A single 32-bit value ...
> >  
> > > +      The function ID follow the ARM SMC calling convention standard [1].  
> >
> >                          follows
> >  
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +required:
> > > +  - compatible
> > > +  - "#mbox-cells"
> > > +
> > > +examples:
> > > +  - |
> > > +    sram@93f000 {
> > > +      compatible = "mmio-sram";
> > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > +      #address-cells = <1>;
> > > +      #size-cells = <1>;
> > > +      ranges = <0x0 0x93f000 0x1000>;
> > > +
> > > +      cpu_scp_lpri: scp-shmem@0 {
> > > +        compatible = "arm,scmi-shmem";
> > > +        reg = <0x0 0x200>;
> > > +      };
> > > +    };
> > > +
> > > +    smc_tx_mbox: tx_mbox {
> > > +      #mbox-cells = <1>;  
> >
> > As mentioned above, should be 0.
> >  
> > > +      compatible = "arm,smc-mbox";
> > > +      /* optional */  
> >
> > First: having "optional" in a specific example is not helpful, just confusing.
> > Second: It is actually *not* optional in this case, as there is no other way of propagating the function ID. The SCMI driver as the mailbox client has certainly no clue about this.
> > I think I said this previously: Relying on the mailbox client to pass the function ID sounds broken, as this is a property of the mailbox controller driver. The mailbox client does not care about this mailbox communication detail, it just wants to trigger the mailbox.
> >  
> Again, the mailbox controller here is the SMC/HVC _instruction_, which
> doesn't care what value the first argument carry.

That is not true. Just check Peng's example implementation he mentioned in the cover letter:
#define FSL_SIP_SCMI_1			0xc20000fe
#define FSL_SIP_SCMI_2			0xc20000ff
....
	case FSL_SIP_SCMI_1:
	case FSL_SIP_SCMI_2:
		SMC_RET1(handle, scmi_handler(smc_fid, x1, x2, x3));

Definitely the function ID is crucial here.

Cheers,
Andre.

Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Andre Przywara]

On Wed, 18 Sep 2019 09:09:25 +0000
Peng Fan <peng.fan@nxp.com> wrote:

Hi,

> > Subject: Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox
> > 
> > On Mon, 16 Sep 2019 09:44:41 +0000
> > Peng Fan <peng.fan@nxp.com> wrote:
> > 
> > Hi,
> > 
> > looks quite good now, some smaller comments below.
> > I think the only thing left is the "function ID passed by the client" topic.
> > 
> > Have you tried using this interface using hvc, for instance in KVM or Xen? For  
> 
> No. I do not have that implementation in hypervisor.

OK, I was thinking about this, because this comes up from to time in Xen, for instance.
It would allow to pass exactly one "virtual" clock to a guest (using SCMI), which Xen could pass on to Dom0, which would translate this to an the actual clock.

I was just worried that having just *one* example implementation might be too little to have a proper generic interface.
I started to update my Allwinner ATF mailbox implementation (which was using SCPI), I will let you know when I managed to eventually test this.

> > instance to provide access to a clock for a passed-through platform device?
> > Another use case that pops up from time to time is GPIO for guests. This
> > sounds like a use case for using the register interface, also we could use the
> > hvc return value.
> >   
> > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7  
> > Cpeng.fa  
> > >  
> > n%40nxp.com%7C58a1ed4078264d14958f08d73b95ed7e%7C686ea1d3bc2b
> > 4c6fa92cd  
> > >  
> > 99c5c301635%7C0%7C1%7C637043387484474825&amp;sdata=Cp1zlhlpQbg
> > BsWu9ZDV  
> > > RKkXmd6kvUR%2BtPO7EPR7YLpA%3D&amp;reserved=0
> > >
> > > Cc: 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 | 167
> > > ++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 176 insertions(+)
> > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
> > >
> > > 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..c84aef39c8d9
> > > --- /dev/null
> > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > @@ -0,0 +1,167 @@
> > > +// 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/interrupt.h>
> > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +struct arm_smc_chan_data {
> > > +	unsigned int function_id;
> > > +};
> > > +
> > > +struct arm_smccc_mbox_cmd {
> > > +	unsigned int function_id;
> > > +	union {
> > > +		unsigned int args_smccc32[6];
> > > +		unsigned long args_smccc64[6];  
> > 
> > Shouldn't this be u32 and u64 here, as the data type has this explicit length in
> > the SMCCC?  
> 
> ok
> 
> >   
> > > +	};
> > > +};  
> > 
> > If this is the data structure that this mailbox controller uses, I would expect
> > this to be documented somewhere, or even exported.  
> 
> Export this structure in include/linux/mailbox/smc-mailbox.h?

For instance, even though I am not sure this is really desired or helpful, since we expect a generic mailbox client (the SCPI or SCMI driver) to deal with that mailbox.

But at least the expected format should be documented, which could just be in writing, not necessarily in code.

> > And again, I don't like the idea of having the function ID in here.  
> 
> You mean function_id in arm_smccc_mbox_cmd is not good?

Yes, this should not be something which mailbox clients are able to choose arbitrarily (see my other email).
 
> >   
> > > +
> > > +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> > > +				    unsigned long, unsigned long,
> > > +				    unsigned long, unsigned long,
> > > +				    unsigned long);
> > > +static 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;
> > > +	u32 function_id;
> > > +
> > > +	function_id = chan_data->function_id;
> > > +	if (!function_id)
> > > +		function_id = cmd->function_id;
> > > +
> > > +	if (function_id & BIT(30)) {  
> > 
> > 	if (ARM_SMCCC_IS_64(function_id)) {  
> 
> ok
> 
> >   
> > > +		ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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 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;
> > > +	u32 function_id = 0;
> > > +
> > > +	if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> > > +		invoke_smc_mbox_fn = __invoke_fn_smc;
> > > +	else
> > > +		invoke_smc_mbox_fn = __invoke_fn_hvc;
> > > +
> > > +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > +	if (!mbox)
> > > +		return -ENOMEM;
> > > +
> > > +	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;
> > > +
> > > +	of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
> > > +	chan_data->function_id = function_id;
> > > +
> > > +	mbox->chans->con_priv = chan_data;
> > > +
> > > +	mbox->txdone_poll = false;
> > > +	mbox->txdone_irq = false;  
> > 
> > Don't we need to provide something to confirm reception to the client? In our
> > case we can set this as soon as the smc/hvc returns.  
> 
> As smc/hvc returns, it means the transfer is over and receive is done.

I understand that, but was wondering if the Linux mailbox framework knows about that? In my older version I was calling mbox_chan_received_data() after the smc call returned.
Also there is mbox_chan_txdone() with which a controller driver can signal TX completion explicitly.

Cheers,
Andre.

> > Cheers,
> > Andre.
> >   
> > > +	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");  
> 

Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Jassi Brar]

On Wed, Sep 18, 2019 at 5:00 AM Andre Przywara <andre.przywara@arm.com> wrote:

> >
> > >
> > > > + };
> > > > +};
> > >
> > > If this is the data structure that this mailbox controller uses, I would expect
> > > this to be documented somewhere, or even exported.
> >
> > Export this structure in include/linux/mailbox/smc-mailbox.h?
>
> For instance, even though I am not sure this is really desired or helpful, since we expect a generic mailbox client (the SCPI or SCMI driver) to deal with that mailbox.
>
> But at least the expected format should be documented, which could just be in writing, not necessarily in code.
>
Yes, the packet format is specified by the controller and defined in
some header for clients to include. Other platforms do that already.



> > > > +
> > > > +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> > > > +                             unsigned long, unsigned long,
> > > > +                             unsigned long, unsigned long,
> > > > +                             unsigned long);
> > > > +static 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;
> > > > + u32 function_id;
> > > > +
> > > > + function_id = chan_data->function_id;
> > > > + if (!function_id)
> > > > +         function_id = cmd->function_id;
> > > > +
> > > > + if (function_id & BIT(30)) {
> > >
> > >     if (ARM_SMCCC_IS_64(function_id)) {
> >
> > ok
> >
> > >
> > > > +         ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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 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;
> > > > + u32 function_id = 0;
> > > > +
> > > > + if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> > > > +         invoke_smc_mbox_fn = __invoke_fn_smc;
> > > > + else
> > > > +         invoke_smc_mbox_fn = __invoke_fn_hvc;
> > > > +
> > > > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > > + if (!mbox)
> > > > +         return -ENOMEM;
> > > > +
> > > > + 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;
> > > > +
> > > > + of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
> > > > + chan_data->function_id = function_id;
> > > > +
> > > > + mbox->chans->con_priv = chan_data;
> > > > +
> > > > + mbox->txdone_poll = false;
> > > > + mbox->txdone_irq = false;
> > >
> > > Don't we need to provide something to confirm reception to the client? In our
> > > case we can set this as soon as the smc/hvc returns.
> >
> > As smc/hvc returns, it means the transfer is over and receive is done.
>
> I understand that, but was wondering if the Linux mailbox framework knows about that? In my older version I was calling mbox_chan_received_data() after the smc call returned.
>
The code already does that at the end of  send_data

> Also there is mbox_chan_txdone() with which a controller driver can signal TX completion explicitly.
>
No. Controller can use that only if it has specified txdone_irq, which
is not the case here.

Thanks

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Jassi Brar]

On Wed, Sep 18, 2019 at 3:53 AM Peng Fan <peng.fan@nxp.com> wrote:

> > >
> > > > +
> > > > +  "#mbox-cells":
> > > > +    const: 1
> > >
> > > Why is this "1"? What is this number used for? It used to be the channel ID,
> > but since you are describing a single channel controller only, this should be 0
> > now.
> > >
> > Yes. I overlooked it and actually queued the patch for pull request.
>
> In Documentation/devicetree/bindings/mailbox/mailbox.txt
> #mbox-cells: Must be at least 1.
>
> So I use 1 here, 0 not work. Because of_mbox_index_xlate expect at least 1 here.
> So I need modify Documentation/devicetree/bindings/mailbox/mailbox.txt
> and add xlate for smc mailbox?
>
No, you just can not use the generic xlate() provided by the api.
Please implement your own xlate() that requires no argument.

Cheers!

RE: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Peng Fan]

> Subject: Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the
> ARM SMC/HVC mailbox
> 
> On Wed, Sep 18, 2019 at 3:53 AM Peng Fan <peng.fan@nxp.com> wrote:
> 
> > > >
> > > > > +
> > > > > +  "#mbox-cells":
> > > > > +    const: 1
> > > >
> > > > Why is this "1"? What is this number used for? It used to be the
> > > > channel ID,
> > > but since you are describing a single channel controller only, this
> > > should be 0 now.
> > > >
> > > Yes. I overlooked it and actually queued the patch for pull request.
> >
> > In Documentation/devicetree/bindings/mailbox/mailbox.txt
> > #mbox-cells: Must be at least 1.
> >
> > So I use 1 here, 0 not work. Because of_mbox_index_xlate expect at least 1
> here.
> > So I need modify Documentation/devicetree/bindings/mailbox/mailbox.txt
> > and add xlate for smc mailbox?
> >
> No, you just can not use the generic xlate() provided by the api.
> Please implement your own xlate() that requires no argument.

ok. Will add xlate.

Thanks,
Peng.

> 
> Cheers!

Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Andre Przywara]

On Wed, 18 Sep 2019 08:31:57 -0500
Jassi Brar <jassisinghbrar@gmail.com> wrote:

Hi,

> On Wed, Sep 18, 2019 at 5:00 AM Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > >  
> > > >  
> > > > > + };
> > > > > +};  
> > > >
> > > > If this is the data structure that this mailbox controller uses, I would expect
> > > > this to be documented somewhere, or even exported.  
> > >
> > > Export this structure in include/linux/mailbox/smc-mailbox.h?  
> >
> > For instance, even though I am not sure this is really desired or helpful, since we expect a generic mailbox client (the SCPI or SCMI driver) to deal with that mailbox.
> >
> > But at least the expected format should be documented, which could just be in writing, not necessarily in code.
> >  
> Yes, the packet format is specified by the controller and defined in
> some header for clients to include. Other platforms do that already.

Yeah, I saw some examples as well, but not every driver was following this apparently.
I guess since we have a fixed data format we should export the struct then, maybe with a remark that the actual usage of registers is up to the protocol (within the SMCCC limits), so is optional.

> > > > > +
> > > > > +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> > > > > +                             unsigned long, unsigned long,
> > > > > +                             unsigned long, unsigned long,
> > > > > +                             unsigned long);
> > > > > +static 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;
> > > > > + u32 function_id;
> > > > > +
> > > > > + function_id = chan_data->function_id;
> > > > > + if (!function_id)
> > > > > +         function_id = cmd->function_id;
> > > > > +
> > > > > + if (function_id & BIT(30)) {  
> > > >
> > > >     if (ARM_SMCCC_IS_64(function_id)) {  
> > >
> > > ok
> > >  
> > > >  
> > > > > +         ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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 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;
> > > > > + u32 function_id = 0;
> > > > > +
> > > > > + if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> > > > > +         invoke_smc_mbox_fn = __invoke_fn_smc;
> > > > > + else
> > > > > +         invoke_smc_mbox_fn = __invoke_fn_hvc;
> > > > > +
> > > > > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > > > + if (!mbox)
> > > > > +         return -ENOMEM;
> > > > > +
> > > > > + 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;
> > > > > +
> > > > > + of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
> > > > > + chan_data->function_id = function_id;
> > > > > +
> > > > > + mbox->chans->con_priv = chan_data;
> > > > > +
> > > > > + mbox->txdone_poll = false;
> > > > > + mbox->txdone_irq = false;  
> > > >
> > > > Don't we need to provide something to confirm reception to the client? In our
> > > > case we can set this as soon as the smc/hvc returns.  
> > >
> > > As smc/hvc returns, it means the transfer is over and receive is done.  
> >
> > I understand that, but was wondering if the Linux mailbox framework knows about that? In my older version I was calling mbox_chan_received_data() after the smc call returned.
> >  
> The code already does that at the end of  send_data

True, for some reason I totally missed that line, sorry for that.
 
> > Also there is mbox_chan_txdone() with which a controller driver can signal TX completion explicitly.
> >  
> No. Controller can use that only if it has specified txdone_irq, which
> is not the case here.

I see. So does the framework handle the case where both txdone_poll and txdone_irq are false?

Cheers,
Andre.

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Jassi Brar]

On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <andre.przywara@arm.com> wrote:

>
> > which needs 9 arguments to work. The fact that the fist argument is
> > always going to be same on a platform is just the way we use this
> > instruction.
> >
> > > We should be as strict as possible to avoid any security issues.
> > >
> > Any example of such a security issue?
>
> Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
>
What if someone finds a way to trick the block layer to erase 'sda' ?
 That is called "bug in the code".
It does happen in every subsystem but we don't stop implementing new
features .... we write flexible code and then fix any bug.


> Do you have any example of a use case where the mailbox client needs to provide the function ID?
>
FSL_SIP_SCMI_1/2 ?
But that is not the main point, which is to be consistent (not
ignoring first argument because someone may find a bug to exploit) and
flexible.


> > > The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
> > >
> > > What would be the use case of this functionality?
> > >
> > At least for flexibility and consistency.
>
> I appreciate the flexibility idea, but when creating an interface, especially a generic one to any kind of firmware, you should be as strict as possible, to avoid clashes in the future.
>
I really don't see how there can be clashes with more complete and
flexible implementation.

Thanks.

Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Jassi Brar]

On Wed, Sep 18, 2019 at 8:58 AM Andre Przywara <andre.przywara@arm.com> wrote:
>

> > > Also there is mbox_chan_txdone() with which a controller driver can signal TX completion explicitly.
> > >
> > No. Controller can use that only if it has specified txdone_irq, which
> > is not the case here.
>
> I see. So does the framework handle the case where both txdone_poll and txdone_irq are false?
>
Of course. If there is no IRQ or POLL mechanism for controller to
detect tx-done, the only way left is for client driver to know by some
'ack' response (if any). The client should call mbox_client_txdone()

Thanks

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Andre Przywara]

On Wed, 18 Sep 2019 09:19:46 -0500
Jassi Brar <jassisinghbrar@gmail.com> wrote:

Hi,

> On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> 
> >  
> > > which needs 9 arguments to work. The fact that the fist argument is
> > > always going to be same on a platform is just the way we use this
> > > instruction.
> > >  
> > > > We should be as strict as possible to avoid any security issues.
> > > >  
> > > Any example of such a security issue?  
> >
> > Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
> >  
> What if someone finds a way to trick the block layer to erase 'sda' ?

Yes, the Linux block driver control the whole block device, it can do whatever it wants.
The firmware provides an interface for multiple users, and the mailbox controller just uses *one part* of it. So we should make sure that it's the right part.

>  That is called "bug in the code".
> It does happen in every subsystem but we don't stop implementing new
> features .... we write flexible code and then fix any bug.
> 
> 
> > Do you have any example of a use case where the mailbox client needs to provide the function ID?
> >  
> FSL_SIP_SCMI_1/2 ?

Huh? Where does the SCPI or SCMI driver provide this? Those clients don't even provide any arguments. Adding some would defeat the whole point of having this mailbox in the first place, which was to provide a drop-in replacement for a hardware mailbox device used on other platforms.

> But that is not the main point, which is to be consistent (not
> ignoring first argument because someone may find a bug to exploit) and
> flexible.

Please read the SMCCC[1]: The first argument is in r1/w1/x1. r0/w0 is the function ID, and this is a specific value (fixed by the firmware implementation, see Peng's ATF patch) and not up to be guessed by a client.

Please keep in mind that we should *NOT* do smc calls without following the SMCCC spec.

> > > > The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
> > > >
> > > > What would be the use case of this functionality?
> > > >  
> > > At least for flexibility and consistency.  
> >
> > I appreciate the flexibility idea, but when creating an interface, especially a generic one to any kind of firmware, you should be as strict as possible, to avoid clashes in the future.
> >  
> I really don't see how there can be clashes with more complete and
> flexible implementation.

Platform A uses PSCI, but no other SMCCC services, so in your scenario you assign a valid function ID say from the SIP range and tell the mailbox *client* to use it. Now you want to run this client on platform B, which happens to have chosen this very function ID for the "set fire on the device" SMCCC service.
So now you would need to assign different IDs to the *client*, depending on the platform? That doesn't make sense. The solution is that you just assign the function ID to the controller, in the (platform specific) DT, so it naturally matches the platform requirements. You can even change it over time, as long as you change both the DT and firmware at the same time.
The client (SCPI, for instance) is totally ignorant about this communication detail, it just needs some mailbox to request services.

That's why I think the function ID (which is part of the SMCCC protocol, not of the mailbox service!) should just be set in the controller DT node and nowhere else.

Cheers,
Andre.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Jassi Brar]

On Wed, Sep 18, 2019 at 9:46 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Wed, 18 Sep 2019 09:19:46 -0500
> Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> Hi,
>
> > On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > >
> > > > which needs 9 arguments to work. The fact that the fist argument is
> > > > always going to be same on a platform is just the way we use this
> > > > instruction.
> > > >
> > > > > We should be as strict as possible to avoid any security issues.
> > > > >
> > > > Any example of such a security issue?
> > >
> > > Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
> > >
> > What if someone finds a way to trick the block layer to erase 'sda' ?
>
> Yes, the Linux block driver control the whole block device, it can do whatever it wants.
>
Sorry, it doesn't make any sense.

> >  That is called "bug in the code".
> > It does happen in every subsystem but we don't stop implementing new
> > features .... we write flexible code and then fix any bug.
> >
> >
> > > Do you have any example of a use case where the mailbox client needs to provide the function ID?
> > >
> > FSL_SIP_SCMI_1/2 ?
>
> Huh? Where does the SCPI or SCMI driver provide this? Those clients don't even provide any arguments. Adding some would defeat the whole point of having this mailbox in the first place, which was to provide a drop-in replacement for a hardware mailbox device used on other platforms.
>
SCPI/SCMI implementation is broken. I did NAK it.
With the 'smc' mailbox you may get away without have to program the
channel before transmit, but not every controller is natively so.

> > But that is not the main point, which is to be consistent (not
> > ignoring first argument because someone may find a bug to exploit) and
> > flexible.
>
> Please read the SMCCC[1]: The first argument is in r1/w1/x1. r0/w0 is the function ID, and this is a specific value (fixed by the firmware implementation, see Peng's ATF patch) and not up to be guessed by a client.
>
The first argument of smc call is the function-id
  arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, 0, &res);


>
> That's why I think the function ID (which is part of the SMCCC protocol, not of the mailbox service!) should just be set in the controller DT node and nowhere else.
>
Actually that is the very reason func-id should be a client thing and
passed via client's DT node :)
It is general understanding that protocol specific bits should not be
a part of controller driver, but the client(protocol) driver.

Page-7 Function-ID specifies :-
1) The service to be invoked.
2) The function to be invoked.
3) The calling convention (32-bit or 64-bit) that is in use.
4) The call type (fast or yielding) that is in use.

Even if we turn blind to 2,3 & 4, but (1) shouts like a runtime property.

Thanks.

Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Andre Przywara]

On Wed, 18 Sep 2019 10:31:57 -0500
Jassi Brar <jassisinghbrar@gmail.com> wrote:

Hi,

> On Wed, Sep 18, 2019 at 9:46 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Wed, 18 Sep 2019 09:19:46 -0500
> > Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > Hi,
> >  
> > > On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> > >  
> > > >  
> > > > > which needs 9 arguments to work. The fact that the fist argument is
> > > > > always going to be same on a platform is just the way we use this
> > > > > instruction.
> > > > >  
> > > > > > We should be as strict as possible to avoid any security issues.
> > > > > >  
> > > > > Any example of such a security issue?  
> > > >
> > > > Someone finds a way to trick some mailbox client to send a crafted message to the mailbox.
> > > >  
> > > What if someone finds a way to trick the block layer to erase 'sda' ?  
> >
> > Yes, the Linux block driver control the whole block device, it can do whatever it wants.
> >  
> Sorry, it doesn't make any sense.

What I meant is that in contrast to a block driver the SMC interface is a shared resource. The mailbox controller is just using a tiny part of that. We must make sure to not interfere with the other services offered by firmware.
 
> > >  That is called "bug in the code".
> > > It does happen in every subsystem but we don't stop implementing new
> > > features .... we write flexible code and then fix any bug.
> > >
> > >  
> > > > Do you have any example of a use case where the mailbox client needs to provide the function ID?
> > > >  
> > > FSL_SIP_SCMI_1/2 ?  
> >
> > Huh? Where does the SCPI or SCMI driver provide this? Those clients don't even provide any arguments. Adding some would defeat the whole point of having this mailbox in the first place, which was to provide a drop-in replacement for a hardware mailbox device used on other platforms.
> >  
> SCPI/SCMI implementation is broken. I did NAK it.
> With the 'smc' mailbox you may get away without have to program the
> channel before transmit, but not every controller is natively so.
> 
> > > But that is not the main point, which is to be consistent (not
> > > ignoring first argument because someone may find a bug to exploit) and
> > > flexible.  
> >
> > Please read the SMCCC[1]: The first argument is in r1/w1/x1. r0/w0 is the function ID, and this is a specific value (fixed by the firmware implementation, see Peng's ATF patch) and not up to be guessed by a client.
> >  
> The first argument of smc call is the function-id
>   arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, 0, &res);

As I wrote we can't safely use a bare smc call. We must comply with the SMCCC, because there are a possible multitude of services the firmware offers. PSCI is a one obvious example.
And SMCCC says:
When an SMC32/HVC32 call is made from AArch32:
   • A Function Identifier is passed in register R0.
   • Arguments are passed in registers R1-R6.
   • Results are returned in R0-R3.
...
(similar for SMC64)

So arguments start from r1/x1.

> >
> > That's why I think the function ID (which is part of the SMCCC protocol, not of the mailbox service!) should just be set in the controller DT node and nowhere else.
> >  
> Actually that is the very reason func-id should be a client thing and
> passed via client's DT node :)
> It is general understanding that protocol specific bits should not be
> a part of controller driver, but the client(protocol) driver.

There are *two* protocols to consider here:
One is the outer SMCCC protocol, which establishes communication between the mailbox controller driver and the firmware. SMCCC defines very precisely the meaning of r0/w0 for that, but leaves the rest (x1-x6) to the user. Think of the function ID being the equivalent of an MMIO base address. You can use it to select several instances of one type of mailbox. It's the task of the controller driver to abstract from that. Surely you wouldn't provide the MMIO base address of the particular mailbox in the client's DT node.

The *client* protocol is then wrapped inside this, using the six argument registers that SMCCC gives us. This is indeed up to the client to use and it dictates its meaning.

Maybe there is some misunderstanding that this mailbox is really a SMCCC mailbox rather than a pure SMC mailbox? Should we use a different naming like smccc_mailbox or firmware_mbox instead?

> Page-7 Function-ID specifies :-
> 1) The service to be invoked.
> 2) The function to be invoked.

"Service" is the term used for a "group of functions". PSCI is one service, for instance. It's typically determined by some upper bits. Inside this service there are several functions, like CPU_ON or SYSTEM_RESET, typically using low order bits.
In our case the service is the mailbox, and there is just one function, the actual doorbell. We could have introduced more functions (like disocvery, information or statistics), but there is no real need for that.

Cheers,
Andre.

> 3) The calling convention (32-bit or 64-bit) that is in use.
> 4) The call type (fast or yielding) that is in use.
> 
> Even if we turn blind to 2,3 & 4, but (1) shouts like a runtime property.

> 
> Thanks.

RE: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Peng Fan]

Hi Jassi,

> Subject: Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Wed, Sep 18, 2019 at 5:00 AM Andre Przywara
> <andre.przywara@arm.com> wrote:
> 
> > >
> > > >
> > > > > + };
> > > > > +};
> > > >
> > > > If this is the data structure that this mailbox controller uses, I
> > > > would expect this to be documented somewhere, or even exported.
> > >
> > > Export this structure in include/linux/mailbox/smc-mailbox.h?
> >
> > For instance, even though I am not sure this is really desired or helpful, since
> we expect a generic mailbox client (the SCPI or SCMI driver) to deal with that
> mailbox.
> >
> > But at least the expected format should be documented, which could just be
> in writing, not necessarily in code.
> >
> Yes, the packet format is specified by the controller and defined in some
> header for clients to include. Other platforms do that already.

So you prefer add the structure in include/linux/mailbox/smc-mailbox.h?

Thanks,
Peng.

> 
> 
> 
> > > > > +
> > > > > +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> > > > > +                             unsigned long, unsigned long,
> > > > > +                             unsigned long, unsigned long,
> > > > > +                             unsigned long); static
> 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;
> > > > > + u32 function_id;
> > > > > +
> > > > > + function_id = chan_data->function_id; if (!function_id)
> > > > > +         function_id = cmd->function_id;
> > > > > +
> > > > > + if (function_id & BIT(30)) {
> > > >
> > > >     if (ARM_SMCCC_IS_64(function_id)) {
> > >
> > > ok
> > >
> > > >
> > > > > +         ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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 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;
> > > > > + u32 function_id = 0;
> > > > > +
> > > > > + if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> > > > > +         invoke_smc_mbox_fn = __invoke_fn_smc; else
> > > > > +         invoke_smc_mbox_fn = __invoke_fn_hvc;
> > > > > +
> > > > > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL); if
> > > > > + (!mbox)
> > > > > +         return -ENOMEM;
> > > > > +
> > > > > + mbox->num_chans = 1;
> > > > > + mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans),
> > > > > + mbox->GFP_KERNEL);
> > > > > + if (!mbox->chans)
> > > > > +         return -ENOMEM;
> > > > > +
> > > > > + chan_data = devm_kzalloc(dev, sizeof(*chan_data), GFP_KERNEL);
> > > > > + if (!chan_data)
> > > > > +         return -ENOMEM;
> > > > > +
> > > > > + of_property_read_u32(dev->of_node, "arm,func-id",
> > > > > + &function_id); chan_data->function_id = function_id;
> > > > > +
> > > > > + mbox->chans->con_priv = chan_data;
> > > > > +
> > > > > + mbox->txdone_poll = false;
> > > > > + mbox->txdone_irq = false;
> > > >
> > > > Don't we need to provide something to confirm reception to the
> > > > client? In our case we can set this as soon as the smc/hvc returns.
> > >
> > > As smc/hvc returns, it means the transfer is over and receive is done.
> >
> > I understand that, but was wondering if the Linux mailbox framework knows
> about that? In my older version I was calling mbox_chan_received_data()
> after the smc call returned.
> >
> The code already does that at the end of  send_data
> 
> > Also there is mbox_chan_txdone() with which a controller driver can signal
> TX completion explicitly.
> >
> No. Controller can use that only if it has specified txdone_irq, which is not the
> case here.
> 
> Thanks