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

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

[Peng Fan]

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

V5:
yaml fix

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       | 125 ++++++++++++
 drivers/mailbox/Kconfig                            |   7 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/arm-smc-mailbox.c                  | 215 +++++++++++++++++++++
 4 files changed, 349 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c

-- 
2.16.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 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       | 125 +++++++++++++++++++++
 1 file changed, 125 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..f8eb28d5e307
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
@@ -0,0 +1,125 @@
+# 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. By nature this operation
+  is synchronous and this mailbox provides no way for asynchronous messages
+  to be delivered the other way round, from firmware to the OS, but
+  asynchronous notification could also be supported. However the value of
+  r0/w0/x0 the firmware returns after the smc call is delivered as a received
+  message to the mailbox framework, so a synchronous communication can be
+  established, for a asynchronous notification, no value will be returned.
+  The exact meaning of both 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:
+    const: arm,smc-mbox
+
+  "#mbox-cells":
+    const: 1
+
+  arm,num-chans:
+    description: The number of channels supported.
+    items:
+      minimum: 1
+      maximum: 4096 # Should be enough?
+
+  method:
+    - enum:
+        - smc
+        - hvc
+
+  transports:
+    - enum:
+        - mem
+        - reg
+
+  arm,func-ids:
+    description: |
+      An array of 32-bit values specifying the function IDs used by each
+      mailbox channel. Those function IDs follow the ARM SMC calling
+      convention standard [1].
+
+      There is one identifier per channel and the number of supported
+      channels is determined by the length of this array.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 0
+    maxItems: 4096   # Should be enough?
+
+required:
+  - compatible
+  - "#mbox-cells"
+  - arm,num-chans
+  - transports
+  - method
+
+examples:
+  - |
+    sram@910000 {
+      compatible = "mmio-sram";
+      reg = <0x0 0x93f000 0x0 0x1000>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      ranges = <0 0x0 0x93f000 0x1000>;
+
+      cpu_scp_lpri: scp-shmem@0 {
+        compatible = "arm,scmi-shmem";
+        reg = <0x0 0x200>;
+      };
+
+      cpu_scp_hpri: scp-shmem@200 {
+        compatible = "arm,scmi-shmem";
+        reg = <0x200 0x200>;
+      };
+    };
+
+    firmware {
+      smc_mbox: mailbox {
+        #mbox-cells = <1>;
+        compatible = "arm,smc-mbox";
+        method = "smc";
+        arm,num-chans = <0x2>;
+        transports = "mem";
+        /* Optional */
+        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
+      };
+
+      scmi {
+        compatible = "arm,scmi";
+        mboxes = <&smc_mbox 0>, <&smc_mbox 1>;
+        mbox-names = "tx", "rx";
+        shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
+      };
+    };
+
+...
-- 
2.16.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 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 | 215 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 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..76a2ae11ee4d
--- /dev/null
+++ b/drivers/mailbox/arm-smc-mailbox.c
@@ -0,0 +1,215 @@
+// 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>
+
+#define ARM_SMC_MBOX_MEM_TRANS	BIT(0)
+
+struct arm_smc_chan_data {
+	u32 function_id;
+	u32 chan_id;
+	u32 flags;
+};
+
+struct arm_smccc_mbox_cmd {
+	unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
+};
+
+typedef unsigned long (smc_mbox_fn)(unsigned long, unsigned long,
+				    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;
+	u32 chan_id;
+
+	if (chan_data->flags & ARM_SMC_MBOX_MEM_TRANS) {
+		if (chan_data->function_id != UINT_MAX)
+			function_id = chan_data->function_id;
+		else
+			function_id = cmd->a0;
+		chan_id = chan_data->chan_id;
+		ret = invoke_smc_mbox_fn(function_id, chan_id, 0, 0, 0, 0,
+					 0, 0);
+	} else {
+		ret = invoke_smc_mbox_fn(cmd->a0, cmd->a1, cmd->a2, cmd->a3,
+					 cmd->a4, cmd->a5, cmd->a6, cmd->a7);
+	}
+
+	mbox_chan_received_data(link, (void *)ret);
+
+	return 0;
+}
+
+static unsigned long __invoke_fn_hvc(unsigned long function_id,
+				     unsigned long arg0, unsigned long arg1,
+				     unsigned long arg2, unsigned long arg3,
+				     unsigned long arg4, unsigned long arg5,
+				     unsigned long arg6)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
+		      arg5, arg6, &res);
+	return res.a0;
+}
+
+static unsigned long __invoke_fn_smc(unsigned long function_id,
+				     unsigned long arg0, unsigned long arg1,
+				     unsigned long arg2, unsigned long arg3,
+				     unsigned long arg4, unsigned long arg5,
+				     unsigned long arg6)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
+		      arg5, arg6, &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;
+	const char *method;
+	bool mem_trans = false;
+	int ret, i;
+	u32 val;
+
+	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
+		if (!val) {
+			dev_err(dev, "invalid arm,num-chans value %u\n", val);
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}
+
+	if (!of_property_read_string(dev->of_node, "transports", &method)) {
+		if (!strcmp("mem", method)) {
+			mem_trans = true;
+		} else if (!strcmp("reg", method)) {
+			mem_trans = false;
+		} else {
+			dev_warn(dev, "invalid \"transports\" property: %s\n",
+				 method);
+
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}
+
+	if (!of_property_read_string(dev->of_node, "method", &method)) {
+		if (!strcmp("hvc", method)) {
+			invoke_smc_mbox_fn = __invoke_fn_hvc;
+		} else if (!strcmp("smc", method)) {
+			invoke_smc_mbox_fn = __invoke_fn_smc;
+		} else {
+			dev_warn(dev, "invalid \"method\" property: %s\n",
+				 method);
+
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	mbox->num_chans = val;
+	mbox->chans = devm_kcalloc(dev, mbox->num_chans, sizeof(*mbox->chans),
+				   GFP_KERNEL);
+	if (!mbox->chans)
+		return -ENOMEM;
+
+	chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
+				 GFP_KERNEL);
+	if (!chan_data)
+		return -ENOMEM;
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		u32 function_id;
+
+		ret = of_property_read_u32_index(dev->of_node,
+						 "arm,func-ids", i,
+						 &function_id);
+		if (ret)
+			chan_data[i].function_id = UINT_MAX;
+
+		else
+			chan_data[i].function_id = function_id;
+
+		chan_data[i].chan_id = i;
+
+		if (mem_trans)
+			chan_data[i].flags |= ARM_SMC_MBOX_MEM_TRANS;
+		mbox->chans[i].con_priv = &chan_data[i];
+	}
+
+	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 with %d chan%s.\n",
+		 mbox->num_chans, mbox->num_chans == 1 ? "" : "s");
+
+	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", },
+	{},
+};
+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("Andre Przywara <andre.przywara@arm.com>");
+MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
+MODULE_LICENSE("GPL v2");
-- 
2.16.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Wed, Aug 28, 2019 at 03:02:58AM +0000, Peng Fan wrote:
> 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       | 125 +++++++++++++++++++++
>  1 file changed, 125 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..f8eb28d5e307
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,125 @@
> +# 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. By nature this operation
> +  is synchronous and this mailbox provides no way for asynchronous messages
> +  to be delivered the other way round, from firmware to the OS, but


> +  asynchronous notification could also be supported.

What do you mean by that ? I would prefer to drop the above line unless
I am missing something. IMO it contradicts the previous statement less
you elaborate more on this.

> However the value of
> +  r0/w0/x0 the firmware returns after the smc call is delivered as a received
> +  message to the mailbox framework, so a synchronous communication can be
> +  established, for a asynchronous notification, no value will be returned.

I assume you refer to asynchronous communication from OS to firmware in the
above statement and "not asynchronous notification" from firmware to OS.

> +  The exact meaning of both 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.
> +

Not sure if reference to SCMI is needed at all but I don't have any
objections to it, just thought worth mentioning.

> +  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.
> +


Other than the above points, I am fine with it. Once fixed,

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Note I haven't reviewed the yaml scheme, but just binding in general.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Wed, Aug 28, 2019 at 03:03:02AM +0000, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> This mailbox driver implements a mailbox which signals transmitted data
> via an ARM smc (secure monitor call) instruction. The mailbox receiver
> is implemented in firmware and can synchronously return data when it
> returns execution to the non-secure world again.
> An asynchronous receive path is not implemented.
> This allows the usage of a mailbox to trigger firmware actions on SoCs
> which either don't have a separate management processor or on which such
> a core is not available. A user of this mailbox could be the SCP
> interface.
>
> 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 | 215 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+)
>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>

[...]

> +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;
> +	const char *method;
> +	bool mem_trans = false;
> +	int ret, i;
> +	u32 val;
> +
> +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> +		if (!val) {
> +			dev_err(dev, "invalid arm,num-chans value %u\n", val);
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	if (!of_property_read_string(dev->of_node, "transports", &method)) {
> +		if (!strcmp("mem", method)) {
> +			mem_trans = true;
> +		} else if (!strcmp("reg", method)) {
> +			mem_trans = false;
> +		} else {
> +			dev_warn(dev, "invalid \"transports\" property: %s\n",
> +				 method);
> +
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> +		if (!strcmp("hvc", method)) {
> +			invoke_smc_mbox_fn = __invoke_fn_hvc;
> +		} else if (!strcmp("smc", method)) {
> +			invoke_smc_mbox_fn = __invoke_fn_smc;
> +		} else {
> +			dev_warn(dev, "invalid \"method\" property: %s\n",
> +				 method);
> +
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (!mbox)
> +		return -ENOMEM;
> +
> +	mbox->num_chans = val;
> +	mbox->chans = devm_kcalloc(dev, mbox->num_chans, sizeof(*mbox->chans),
> +				   GFP_KERNEL);
> +	if (!mbox->chans)
> +		return -ENOMEM;
> +
> +	chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
> +				 GFP_KERNEL);
> +	if (!chan_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < mbox->num_chans; i++) {
> +		u32 function_id;
> +
> +		ret = of_property_read_u32_index(dev->of_node,
> +						 "arm,func-ids", i,
> +						 &function_id);

I missed it in binding but I thought we agreed to make this "arm,func-ids"
a required property and not optional ?

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Wed, Aug 28, 2019 at 03:02:58AM +0000, Peng Fan wrote:
> > 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       | 125
> +++++++++++++++++++++
> >  1 file changed, 125 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..f8eb28d5e307
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,125 @@
> > +# 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%7C37aa729c94944730868b08d72bbfc121%7C686ea1
> d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C1%7C637025974936865698&amp;sdata=Inp
> %2FLs39m
> > +Gv1fe3dZMSaGmgmyWPT6awPh47s3mEtQ%2BQ%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%7C37aa729c94944730868b08d72bbfc121%7C686ea1d3bc2b4c
> 6fa92cd9
> >
> +9c5c301635%7C0%7C1%7C637025974936865698&amp;sdata=jmoR1Qqm7
> 6N5NwDbgFE
> > +Fm8cpdW%2B%2FgqmG9mSGz9mXv58%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
> > +  call) instruction to trigger a mailbox-connected activity in
> > +firmware,
> > +  executing on the very same core as the caller. By nature this
> > +operation
> > +  is synchronous and this mailbox provides no way for asynchronous
> > +messages
> > +  to be delivered the other way round, from firmware to the OS, but
> 
> 
> > +  asynchronous notification could also be supported.
> 
> What do you mean by that ? I would prefer to drop the above line unless I am

Ok. Dropped it in v6.

> missing something. IMO it contradicts the previous statement less you
> elaborate more on this.
> 
> > However the value of
> > +  r0/w0/x0 the firmware returns after the smc call is delivered as a
> > + received  message to the mailbox framework, so a synchronous
> > + communication can be  established, for a asynchronous notification, no
> value will be returned.
> 
> I assume you refer to asynchronous communication from OS to firmware in
> the above statement and "not asynchronous notification" from firmware to
> OS.

Since asynchronous notification dropped, so it should only be
synchronous communication could be established. So I'll
modify it as below:

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 both 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.
> > +
> 
> Not sure if reference to SCMI is needed at all but I don't have any objections
> to it, just thought worth mentioning.
> 
> > +  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.
> > +
> 
> 
> Other than the above points, I am fine with it. Once fixed,
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks,
Peng.

> 
> Note I haven't reviewed the yaml scheme, but just binding in general.
> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v5 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Wed, Aug 28, 2019 at 03:03:02AM +0000, Peng Fan wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > This mailbox driver implements a mailbox which signals transmitted
> > data via an ARM smc (secure monitor call) instruction. The mailbox
> > receiver is implemented in firmware and can synchronously return data
> > when it returns execution to the non-secure world again.
> > An asynchronous receive path is not implemented.
> > This allows the usage of a mailbox to trigger firmware actions on SoCs
> > which either don't have a separate management processor or on which
> > such a core is not available. A user of this mailbox could be the SCP
> > interface.
> >
> > 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%7Ca1e96c6b782d43b2cfb208d72bc05898%7C686ea1d3bc2b
> 4c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C637025977487779923&amp;sdata=rzC%2B4Y1c
> q9Y3tSDFR
> > %2Fsvf5ktk7INP2rwXN%2BXdWCVjNs%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 | 215
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 224 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
> >
> 
> [...]
> 
> > +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;
> > +	const char *method;
> > +	bool mem_trans = false;
> > +	int ret, i;
> > +	u32 val;
> > +
> > +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> > +		if (!val) {
> > +			dev_err(dev, "invalid arm,num-chans value %u\n", val);
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!of_property_read_string(dev->of_node, "transports", &method)) {
> > +		if (!strcmp("mem", method)) {
> > +			mem_trans = true;
> > +		} else if (!strcmp("reg", method)) {
> > +			mem_trans = false;
> > +		} else {
> > +			dev_warn(dev, "invalid \"transports\" property: %s\n",
> > +				 method);
> > +
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> > +		if (!strcmp("hvc", method)) {
> > +			invoke_smc_mbox_fn = __invoke_fn_hvc;
> > +		} else if (!strcmp("smc", method)) {
> > +			invoke_smc_mbox_fn = __invoke_fn_smc;
> > +		} else {
> > +			dev_warn(dev, "invalid \"method\" property: %s\n",
> > +				 method);
> > +
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > +	if (!mbox)
> > +		return -ENOMEM;
> > +
> > +	mbox->num_chans = val;
> > +	mbox->chans = devm_kcalloc(dev, mbox->num_chans,
> sizeof(*mbox->chans),
> > +				   GFP_KERNEL);
> > +	if (!mbox->chans)
> > +		return -ENOMEM;
> > +
> > +	chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
> > +				 GFP_KERNEL);
> > +	if (!chan_data)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < mbox->num_chans; i++) {
> > +		u32 function_id;
> > +
> > +		ret = of_property_read_u32_index(dev->of_node,
> > +						 "arm,func-ids", i,
> > +						 &function_id);
> 
> I missed it in binding but I thought we agreed to make this "arm,func-ids"
> a required property and not optional ?

Not sure Jassi is fine with it being a required property, but I could convert
it to a required property in V6.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Tue, Aug 27, 2019 at 10:02 PM Peng Fan <peng.fan@nxp.com> wrote:
>
> 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       | 125 +++++++++++++++++++++
>  1 file changed, 125 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..f8eb28d5e307
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,125 @@
> +# 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. By nature this operation
> +  is synchronous and this mailbox provides no way for asynchronous messages
> +  to be delivered the other way round, from firmware to the OS, but
> +  asynchronous notification could also be supported. However the value of
> +  r0/w0/x0 the firmware returns after the smc call is delivered as a received
> +  message to the mailbox framework, so a synchronous communication can be
> +  established, for a asynchronous notification, no value will be returned.
> +  The exact meaning of both 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:
> +    const: arm,smc-mbox
> +
> +  "#mbox-cells":
> +    const: 1
> +
> +  arm,num-chans:
> +    description: The number of channels supported.
> +    items:
> +      minimum: 1
> +      maximum: 4096 # Should be enough?
> +
> +  method:
> +    - enum:
> +        - smc
> +        - hvc
> +
> +  transports:
> +    - enum:
> +        - mem
> +        - reg
> +
> +  arm,func-ids:
> +    description: |
> +      An array of 32-bit values specifying the function IDs used by each
> +      mailbox channel. Those function IDs follow the ARM SMC calling
> +      convention standard [1].
> +
> +      There is one identifier per channel and the number of supported
> +      channels is determined by the length of this array.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 0
> +    maxItems: 4096   # Should be enough?
> +
> +required:
> +  - compatible
> +  - "#mbox-cells"
> +  - arm,num-chans
> +  - transports
> +  - method
> +
> +examples:
> +  - |
> +    sram@910000 {
> +      compatible = "mmio-sram";
> +      reg = <0x0 0x93f000 0x0 0x1000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      ranges = <0 0x0 0x93f000 0x1000>;
> +
> +      cpu_scp_lpri: scp-shmem@0 {
> +        compatible = "arm,scmi-shmem";
> +        reg = <0x0 0x200>;
> +      };
> +
> +      cpu_scp_hpri: scp-shmem@200 {
> +        compatible = "arm,scmi-shmem";
> +        reg = <0x200 0x200>;
> +      };
> +    };
> +
> +    firmware {
> +      smc_mbox: mailbox {
> +        #mbox-cells = <1>;
> +        compatible = "arm,smc-mbox";
> +        method = "smc";
> +        arm,num-chans = <0x2>;
> +        transports = "mem";
> +        /* Optional */
> +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
>
SMC/HVC is synchronously(block) running in "secure mode", i.e, there
can only be one instance running platform wide. Right?  That implies
there is only one physical channel in the platform. So if you need to
initiate different functions (tx, rx), you call them sequentially by
changing the func-id for each request. Why not?

-Jassi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Jassi,

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Tue, Aug 27, 2019 at 10:02 PM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > 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       | 125
> +++++++++++++++++++++
> >  1 file changed, 125 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..f8eb28d5e307
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,125 @@
> > +# 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%7C8aa671dfa4d04ba003b508d72d0f297f%7C686ea1d
> 3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C1%7C637027415448196145&amp;sdata=xd
> nUObNqlRF
> > +lu8NiXSuc35fYrHIzR%2Fyak6IzW05Q3nA%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%7C8aa671dfa4d04ba003b508d72d0f297f%7C686ea1d3bc2b4c6
> fa92cd9
> >
> +9c5c301635%7C0%7C1%7C637027415448196145&amp;sdata=wl%2Fdg09
> QMS%2FoHgI
> > +yD7ZBNpoIGXYxfFDRWhyYHogFd6A%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
> > +  call) instruction to trigger a mailbox-connected activity in
> > +firmware,
> > +  executing on the very same core as the caller. By nature this
> > +operation
> > +  is synchronous and this mailbox provides no way for asynchronous
> > +messages
> > +  to be delivered the other way round, from firmware to the OS, but
> > +  asynchronous notification could also be supported. However the
> > +value of
> > +  r0/w0/x0 the firmware returns after the smc call is delivered as a
> > +received
> > +  message to the mailbox framework, so a synchronous communication
> > +can be
> > +  established, for a asynchronous notification, no value will be returned.
> > +  The exact meaning of both 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:
> > +    const: arm,smc-mbox
> > +
> > +  "#mbox-cells":
> > +    const: 1
> > +
> > +  arm,num-chans:
> > +    description: The number of channels supported.
> > +    items:
> > +      minimum: 1
> > +      maximum: 4096 # Should be enough?
> > +
> > +  method:
> > +    - enum:
> > +        - smc
> > +        - hvc
> > +
> > +  transports:
> > +    - enum:
> > +        - mem
> > +        - reg
> > +
> > +  arm,func-ids:
> > +    description: |
> > +      An array of 32-bit values specifying the function IDs used by each
> > +      mailbox channel. Those function IDs follow the ARM SMC calling
> > +      convention standard [1].
> > +
> > +      There is one identifier per channel and the number of supported
> > +      channels is determined by the length of this array.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 0
> > +    maxItems: 4096   # Should be enough?
> > +
> > +required:
> > +  - compatible
> > +  - "#mbox-cells"
> > +  - arm,num-chans
> > +  - transports
> > +  - method
> > +
> > +examples:
> > +  - |
> > +    sram@910000 {
> > +      compatible = "mmio-sram";
> > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +      ranges = <0 0x0 0x93f000 0x1000>;
> > +
> > +      cpu_scp_lpri: scp-shmem@0 {
> > +        compatible = "arm,scmi-shmem";
> > +        reg = <0x0 0x200>;
> > +      };
> > +
> > +      cpu_scp_hpri: scp-shmem@200 {
> > +        compatible = "arm,scmi-shmem";
> > +        reg = <0x200 0x200>;
> > +      };
> > +    };
> > +
> > +    firmware {
> > +      smc_mbox: mailbox {
> > +        #mbox-cells = <1>;
> > +        compatible = "arm,smc-mbox";
> > +        method = "smc";
> > +        arm,num-chans = <0x2>;
> > +        transports = "mem";
> > +        /* Optional */
> > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> >
> SMC/HVC is synchronously(block) running in "secure mode", i.e, there can
> only be one instance running platform wide. Right?

I think there could be channel for TEE, and channel for Linux.
For virtualization case, there could be dedicated channel for each VM.

  That implies there is only
> one physical channel in the platform.

I don't think so, TEE/Linux should use different physical channels,
i.e, SRAM memory partitioned using TZASC.

 So if you need to initiate different
> functions (tx, rx), you call them sequentially by changing the func-id for each
> request. Why not?

I could not follow you clearly. Could you please share more details?

Thanks,
Peng.
> 
> -Jassi
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:

> > > +examples:
> > > +  - |
> > > +    sram@910000 {
> > > +      compatible = "mmio-sram";
> > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > +      #address-cells = <1>;
> > > +      #size-cells = <1>;
> > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > +
> > > +      cpu_scp_lpri: scp-shmem@0 {
> > > +        compatible = "arm,scmi-shmem";
> > > +        reg = <0x0 0x200>;
> > > +      };
> > > +
> > > +      cpu_scp_hpri: scp-shmem@200 {
> > > +        compatible = "arm,scmi-shmem";
> > > +        reg = <0x200 0x200>;
> > > +      };
> > > +    };
> > > +
> > > +    firmware {
> > > +      smc_mbox: mailbox {
> > > +        #mbox-cells = <1>;
> > > +        compatible = "arm,smc-mbox";
> > > +        method = "smc";
> > > +        arm,num-chans = <0x2>;
> > > +        transports = "mem";
> > > +        /* Optional */
> > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > >
> > SMC/HVC is synchronously(block) running in "secure mode", i.e, there can
> > only be one instance running platform wide. Right?
>
> I think there could be channel for TEE, and channel for Linux.
> For virtualization case, there could be dedicated channel for each VM.
>
I am talking from Linux pov. Functions 0xfe and 0xff above, can't both
be active at the same time, right?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Jassi,

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> 
> > > > +examples:
> > > > +  - |
> > > > +    sram@910000 {
> > > > +      compatible = "mmio-sram";
> > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > +      #address-cells = <1>;
> > > > +      #size-cells = <1>;
> > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > +
> > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > +        compatible = "arm,scmi-shmem";
> > > > +        reg = <0x0 0x200>;
> > > > +      };
> > > > +
> > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > +        compatible = "arm,scmi-shmem";
> > > > +        reg = <0x200 0x200>;
> > > > +      };
> > > > +    };
> > > > +
> > > > +    firmware {
> > > > +      smc_mbox: mailbox {
> > > > +        #mbox-cells = <1>;
> > > > +        compatible = "arm,smc-mbox";
> > > > +        method = "smc";
> > > > +        arm,num-chans = <0x2>;
> > > > +        transports = "mem";
> > > > +        /* Optional */
> > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > >
> > > SMC/HVC is synchronously(block) running in "secure mode", i.e, there
> > > can only be one instance running platform wide. Right?
> >
> > I think there could be channel for TEE, and channel for Linux.
> > For virtualization case, there could be dedicated channel for each VM.
> >
> I am talking from Linux pov. Functions 0xfe and 0xff above, can't both be
> active at the same time, right?

If I get your point correctly,
On UP, both could not be active. On SMP, tx/rx could be both active, anyway
this depends on secure firmware and Linux firmware design.

Do you have any suggestions about arm,func-ids here?

Thanks,
Peng.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Jassi,
>
> > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> > SMC/HVC mailbox
> >
> > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > > > > +examples:
> > > > > +  - |
> > > > > +    sram@910000 {
> > > > > +      compatible = "mmio-sram";
> > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > +      #address-cells = <1>;
> > > > > +      #size-cells = <1>;
> > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > +
> > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > +        compatible = "arm,scmi-shmem";
> > > > > +        reg = <0x0 0x200>;
> > > > > +      };
> > > > > +
> > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > +        compatible = "arm,scmi-shmem";
> > > > > +        reg = <0x200 0x200>;
> > > > > +      };
> > > > > +    };
> > > > > +
> > > > > +    firmware {
> > > > > +      smc_mbox: mailbox {
> > > > > +        #mbox-cells = <1>;
> > > > > +        compatible = "arm,smc-mbox";
> > > > > +        method = "smc";
> > > > > +        arm,num-chans = <0x2>;
> > > > > +        transports = "mem";
> > > > > +        /* Optional */
> > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > >
> > > > SMC/HVC is synchronously(block) running in "secure mode", i.e, there
> > > > can only be one instance running platform wide. Right?
> > >
> > > I think there could be channel for TEE, and channel for Linux.
> > > For virtualization case, there could be dedicated channel for each VM.
> > >
> > I am talking from Linux pov. Functions 0xfe and 0xff above, can't both be
> > active at the same time, right?
>
> If I get your point correctly,
> On UP, both could not be active. On SMP, tx/rx could be both active, anyway
> this depends on secure firmware and Linux firmware design.
>
> Do you have any suggestions about arm,func-ids here?
>
I was thinking if this is just an instruction, why can't each channel
be represented as a controller, i.e, have exactly one func-id per
controller node. Define as many controllers as you need channels ?

-j

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > Hi Jassi,
> >
> > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > for the ARM SMC/HVC mailbox
> > >
> > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> > >
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    sram@910000 {
> > > > > > +      compatible = "mmio-sram";
> > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > +      #address-cells = <1>;
> > > > > > +      #size-cells = <1>;
> > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > +
> > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > +        reg = <0x0 0x200>;
> > > > > > +      };
> > > > > > +
> > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > +        reg = <0x200 0x200>;
> > > > > > +      };
> > > > > > +    };
> > > > > > +
> > > > > > +    firmware {
> > > > > > +      smc_mbox: mailbox {
> > > > > > +        #mbox-cells = <1>;
> > > > > > +        compatible = "arm,smc-mbox";
> > > > > > +        method = "smc";
> > > > > > +        arm,num-chans = <0x2>;
> > > > > > +        transports = "mem";
> > > > > > +        /* Optional */
> > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > >
> > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > there can only be one instance running platform wide. Right?
> > > >
> > > > I think there could be channel for TEE, and channel for Linux.
> > > > For virtualization case, there could be dedicated channel for each VM.
> > > >
> > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > both be active at the same time, right?
> >
> > If I get your point correctly,
> > On UP, both could not be active. On SMP, tx/rx could be both active,
> > anyway this depends on secure firmware and Linux firmware design.
> >
> > Do you have any suggestions about arm,func-ids here?
> >
> I was thinking if this is just an instruction, why can't each channel be
> represented as a controller, i.e, have exactly one func-id per controller node.
> Define as many controllers as you need channels ?

I am ok, this could make driver code simpler. Something as below?

    smc_tx_mbox: tx_mbox {
      #mbox-cells = <0>;
      compatible = "arm,smc-mbox";
      method = "smc";
      transports = "mem";
      arm,func-id = <0xc20000fe>;
    };

    smc_rx_mbox: rx_mbox {
      #mbox-cells = <0>;
      compatible = "arm,smc-mbox";
      method = "smc";
      transports = "mem";
      arm,func-id = <0xc20000ff>;
    };

    firmware {
      scmi {
        compatible = "arm,scmi";
        mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
        mbox-names = "tx", "rx";
        shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
      };
    };

Thanks,
Peng.

> 
> -j
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Fri, Aug 30, 2019 at 3:07 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> > SMC/HVC mailbox
> >
> > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:
> > >
> > > Hi Jassi,
> > >
> > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > for the ARM SMC/HVC mailbox
> > > >
> > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > >
> > > > > > > +examples:
> > > > > > > +  - |
> > > > > > > +    sram@910000 {
> > > > > > > +      compatible = "mmio-sram";
> > > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > +      #address-cells = <1>;
> > > > > > > +      #size-cells = <1>;
> > > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > +
> > > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > +        reg = <0x0 0x200>;
> > > > > > > +      };
> > > > > > > +
> > > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > +        reg = <0x200 0x200>;
> > > > > > > +      };
> > > > > > > +    };
> > > > > > > +
> > > > > > > +    firmware {
> > > > > > > +      smc_mbox: mailbox {
> > > > > > > +        #mbox-cells = <1>;
> > > > > > > +        compatible = "arm,smc-mbox";
> > > > > > > +        method = "smc";
> > > > > > > +        arm,num-chans = <0x2>;
> > > > > > > +        transports = "mem";
> > > > > > > +        /* Optional */
> > > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > > >
> > > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > > there can only be one instance running platform wide. Right?
> > > > >
> > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > For virtualization case, there could be dedicated channel for each VM.
> > > > >
> > > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > > both be active at the same time, right?
> > >
> > > If I get your point correctly,
> > > On UP, both could not be active. On SMP, tx/rx could be both active,
> > > anyway this depends on secure firmware and Linux firmware design.
> > >
> > > Do you have any suggestions about arm,func-ids here?
> > >
> > I was thinking if this is just an instruction, why can't each channel be
> > represented as a controller, i.e, have exactly one func-id per controller node.
> > Define as many controllers as you need channels ?
>
> I am ok, this could make driver code simpler. Something as below?
>
>     smc_tx_mbox: tx_mbox {
>       #mbox-cells = <0>;
>       compatible = "arm,smc-mbox";
>       method = "smc";
>       transports = "mem";
>       arm,func-id = <0xc20000fe>;
>     };
>
>     smc_rx_mbox: rx_mbox {
>       #mbox-cells = <0>;
>       compatible = "arm,smc-mbox";
>       method = "smc";
>       transports = "mem";
>       arm,func-id = <0xc20000ff>;
>     };
>
>     firmware {
>       scmi {
>         compatible = "arm,scmi";
>         mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
>         mbox-names = "tx", "rx";
>         shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
>       };
>     };
>
Yes, the channel part is good.
But I am not convinced by the need to have SCMI specific "transport" mode.

thanks

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Fri, Aug 30, 2019 at 3:07 AM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > for the ARM SMC/HVC mailbox
> > >
> > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > >
> > > > Hi Jassi,
> > > >
> > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding
> > > > > doc for the ARM SMC/HVC mailbox
> > > > >
> > > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > > >
> > > > > > > > +examples:
> > > > > > > > +  - |
> > > > > > > > +    sram@910000 {
> > > > > > > > +      compatible = "mmio-sram";
> > > > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > > +      #address-cells = <1>;
> > > > > > > > +      #size-cells = <1>;
> > > > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > > +
> > > > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > +        reg = <0x0 0x200>;
> > > > > > > > +      };
> > > > > > > > +
> > > > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > +        reg = <0x200 0x200>;
> > > > > > > > +      };
> > > > > > > > +    };
> > > > > > > > +
> > > > > > > > +    firmware {
> > > > > > > > +      smc_mbox: mailbox {
> > > > > > > > +        #mbox-cells = <1>;
> > > > > > > > +        compatible = "arm,smc-mbox";
> > > > > > > > +        method = "smc";
> > > > > > > > +        arm,num-chans = <0x2>;
> > > > > > > > +        transports = "mem";
> > > > > > > > +        /* Optional */
> > > > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > > > >
> > > > > > > SMC/HVC is synchronously(block) running in "secure mode",
> > > > > > > i.e, there can only be one instance running platform wide. Right?
> > > > > >
> > > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > > For virtualization case, there could be dedicated channel for each
> VM.
> > > > > >
> > > > > I am talking from Linux pov. Functions 0xfe and 0xff above,
> > > > > can't both be active at the same time, right?
> > > >
> > > > If I get your point correctly,
> > > > On UP, both could not be active. On SMP, tx/rx could be both
> > > > active, anyway this depends on secure firmware and Linux firmware
> design.
> > > >
> > > > Do you have any suggestions about arm,func-ids here?
> > > >
> > > I was thinking if this is just an instruction, why can't each
> > > channel be represented as a controller, i.e, have exactly one func-id per
> controller node.
> > > Define as many controllers as you need channels ?
> >
> > I am ok, this could make driver code simpler. Something as below?
> >
> >     smc_tx_mbox: tx_mbox {
> >       #mbox-cells = <0>;
> >       compatible = "arm,smc-mbox";
> >       method = "smc";
> >       transports = "mem";
> >       arm,func-id = <0xc20000fe>;
> >     };
> >
> >     smc_rx_mbox: rx_mbox {
> >       #mbox-cells = <0>;
> >       compatible = "arm,smc-mbox";
> >       method = "smc";
> >       transports = "mem";
> >       arm,func-id = <0xc20000ff>;
> >     }
> >
> >     firmware {
> >       scmi {
> >         compatible = "arm,scmi";
> >         mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
> >         mbox-names = "tx", "rx";
> >         shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> >       };
> >     };
> >
> Yes, the channel part is good.
> But I am not convinced by the need to have SCMI specific "transport" mode.

SCMI spec only support shared memory message. However to make this driver
generic, need to take care of message using ARM registers.

If using shared memory message, the call will be
invoke_smc_mbox_fn(function_id, chan_id, 0, 0, 0, 0, 0, 0);
If using ARM registers to transfer message, the call will be
invoke_smc_mbox_fn(cmd->a0, cmd->a1, cmd->a2, cmd->a3, 
cmd->a4, cmd->a5, cmd->a6, cmd->a7);

So I added "transports" mode.

Code as below:
        if (chan_data->flags & ARM_SMC_MBOX_MEM_TRANS) {
                if (chan_data->function_id != UINT_MAX)
                        function_id = chan_data->function_id;
                else
                        function_id = cmd->a0;
                chan_id = chan_data->chan_id;
                ret = invoke_smc_mbox_fn(function_id, chan_id, 0, 0, 0, 0,
                                         0, 0);
        } else {
                ret = invoke_smc_mbox_fn(cmd->a0, cmd->a1, cmd->a2, cmd->a3,
                                         cmd->a4, cmd->a5, cmd->a6, cmd->a7);
        }


Per Sudeep's comments in previous version, better pass chan_id
to secure firmware.
If drop the "transports" mode, I do not have a good idea how to differentiate
the two cases, reg and mem. Any suggestions?

Thanks,
Peng.


> 
> thanks
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Fri, Aug 30, 2019 at 07:37:41AM +0000, Peng Fan wrote:
> Hi Jassi,
> > > I think there could be channel for TEE, and channel for Linux.
> > > For virtualization case, there could be dedicated channel for each VM.
> > >
> > I am talking from Linux pov. Functions 0xfe and 0xff above, can't both be
> > active at the same time, right?
>
> If I get your point correctly,
> On UP, both could not be active. On SMP, tx/rx could be both active, anyway
> this depends on secure firmware and Linux firmware design.
>

Just to confirm, we can't have SMC/HVC based Rx channel as there's no
*architectural* way to achieve it. So it can be based on some interrupt
from secure side and hence will be a *different* type of channel/controller.

Sorry to make this point repeatedly, but juts to be absolutely clear:
as it stands, SMC/HVC can be used only for Tx today.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Fri, Aug 30, 2019 at 02:52:40AM -0500, Jassi Brar wrote:
> On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:

[...]

> >
> > If I get your point correctly,
> > On UP, both could not be active. On SMP, tx/rx could be both active, anyway
> > this depends on secure firmware and Linux firmware design.
> >
> > Do you have any suggestions about arm,func-ids here?
> >
> I was thinking if this is just an instruction, why can't each channel
> be represented as a controller, i.e, have exactly one func-id per
> controller node. Define as many controllers as you need channels ?
>

I might have missed to follow this, but what's the advantage of doing so ?
Which can't single controller instance deal with all the channels ?

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Sudeep

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Fri, Aug 30, 2019 at 07:37:41AM +0000, Peng Fan wrote:
> > Hi Jassi,
> > > > I think there could be channel for TEE, and channel for Linux.
> > > > For virtualization case, there could be dedicated channel for each VM.
> > > >
> > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > both be active at the same time, right?
> >
> > If I get your point correctly,
> > On UP, both could not be active. On SMP, tx/rx could be both active,
> > anyway this depends on secure firmware and Linux firmware design.
> >
> 
> Just to confirm, we can't have SMC/HVC based Rx channel as there's no
> *architectural* way to achieve it. So it can be based on some interrupt from
> secure side and hence will be a *different* type of channel/controller.
> 
> Sorry to make this point repeatedly, but juts to be absolutely clear:
> as it stands, SMC/HVC can be used only for Tx today.

Since interrupt notification was dropped in v5, I need to drop RX description
in v6.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Fri, Aug 30, 2019 at 4:32 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Aug 30, 2019 at 02:52:40AM -0500, Jassi Brar wrote:
> > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> [...]
>
> > >
> > > If I get your point correctly,
> > > On UP, both could not be active. On SMP, tx/rx could be both active, anyway
> > > this depends on secure firmware and Linux firmware design.
> > >
> > > Do you have any suggestions about arm,func-ids here?
> > >
> > I was thinking if this is just an instruction, why can't each channel
> > be represented as a controller, i.e, have exactly one func-id per
> > controller node. Define as many controllers as you need channels ?
> >
>
> I might have missed to follow this, but what's the advantage of doing so ?
> Which can't single controller instance deal with all the channels ?
>
There are many advantages ...
1) Design reflects the reality - two smc/hvc instructions have nothing
tying them together.
2) Driver code becomes simpler - don't have to pre-populate channels,
deducting from the size of func-ids array.
3) Driver becomes more flexible - We can have channels that pass
func-id runtime and channels that pass via DT (if we must have the
option of DT property).

-jassi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Wed, Aug 28, 2019 at 03:02:58AM +0000, Peng Fan wrote:
> 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       | 125 +++++++++++++++++++++
>  1 file changed, 125 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..f8eb28d5e307
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,125 @@
> +# 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. By nature this operation
> +  is synchronous and this mailbox provides no way for asynchronous messages
> +  to be delivered the other way round, from firmware to the OS, but
> +  asynchronous notification could also be supported. However the value of
> +  r0/w0/x0 the firmware returns after the smc call is delivered as a received
> +  message to the mailbox framework, so a synchronous communication can be
> +  established, for a asynchronous notification, no value will be returned.
> +  The exact meaning of both 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:
> +    const: arm,smc-mbox
> +
> +  "#mbox-cells":
> +    const: 1
> +
> +  arm,num-chans:
> +    description: The number of channels supported.
> +    items:
> +      minimum: 1
> +      maximum: 4096 # Should be enough?
> +
> +  method:
> +    - enum:

Did you build this with 'make dt_binding_check' as this should be a 
warning. This should not be a list entry (i.e. drop the '-').

> +        - smc
> +        - hvc
> +
> +  transports:

arm,transports

> +    - enum:
> +        - mem
> +        - reg
> +
> +  arm,func-ids:
> +    description: |
> +      An array of 32-bit values specifying the function IDs used by each
> +      mailbox channel. Those function IDs follow the ARM SMC calling
> +      convention standard [1].
> +
> +      There is one identifier per channel and the number of supported
> +      channels is determined by the length of this array.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 0
> +    maxItems: 4096   # Should be enough?
> +
> +required:
> +  - compatible
> +  - "#mbox-cells"
> +  - arm,num-chans
> +  - transports
> +  - method
> +
> +examples:
> +  - |
> +    sram@910000 {
> +      compatible = "mmio-sram";
> +      reg = <0x0 0x93f000 0x0 0x1000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      ranges = <0 0x0 0x93f000 0x1000>;
> +
> +      cpu_scp_lpri: scp-shmem@0 {
> +        compatible = "arm,scmi-shmem";
> +        reg = <0x0 0x200>;
> +      };
> +
> +      cpu_scp_hpri: scp-shmem@200 {
> +        compatible = "arm,scmi-shmem";
> +        reg = <0x200 0x200>;
> +      };
> +    };
> +
> +    firmware {
> +      smc_mbox: mailbox {
> +        #mbox-cells = <1>;
> +        compatible = "arm,smc-mbox";
> +        method = "smc";
> +        arm,num-chans = <0x2>;
> +        transports = "mem";
> +        /* Optional */
> +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> +      };
> +
> +      scmi {
> +        compatible = "arm,scmi";
> +        mboxes = <&smc_mbox 0>, <&smc_mbox 1>;
> +        mbox-names = "tx", "rx";
> +        shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> +      };
> +    };
> +
> +...
> -- 
> 2.16.4
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Mon, Sep 2, 2019 at 2:39 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Aug 28, 2019 at 03:02:58AM +0000, Peng Fan wrote:
> > 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       | 125 +++++++++++++++++++++
> >  1 file changed, 125 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..f8eb28d5e307
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,125 @@
> > +# 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. By nature this operation
> > +  is synchronous and this mailbox provides no way for asynchronous messages
> > +  to be delivered the other way round, from firmware to the OS, but
> > +  asynchronous notification could also be supported. However the value of
> > +  r0/w0/x0 the firmware returns after the smc call is delivered as a received
> > +  message to the mailbox framework, so a synchronous communication can be
> > +  established, for a asynchronous notification, no value will be returned.
> > +  The exact meaning of both 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:
> > +    const: arm,smc-mbox
> > +
> > +  "#mbox-cells":
> > +    const: 1
> > +
> > +  arm,num-chans:
> > +    description: The number of channels supported.
> > +    items:
> > +      minimum: 1
> > +      maximum: 4096 # Should be enough?
> > +
> > +  method:
> > +    - enum:
>
> Did you build this with 'make dt_binding_check' as this should be a
> warning. This should not be a list entry (i.e. drop the '-').
>
> > +        - smc
> > +        - hvc
> > +
> > +  transports:
>
> arm,transports
>
> > +    - enum:
> > +        - mem
> > +        - reg
> > +
> > +  arm,func-ids:
> > +    description: |
> > +      An array of 32-bit values specifying the function IDs used by each
> > +      mailbox channel. Those function IDs follow the ARM SMC calling
> > +      convention standard [1].
> > +
> > +      There is one identifier per channel and the number of supported
> > +      channels is determined by the length of this array.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array

Also, this doesn't work. You need:

allOf:
  - $ref: /schemas/types.yaml#/definitions/uint32-array

> > +    minItems: 0
> > +    maxItems: 4096   # Should be enough?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andre Przywara]

On Fri, 30 Aug 2019 03:12:29 -0500
Jassi Brar <jassisinghbrar@gmail.com> wrote:

Hi,

> On Fri, Aug 30, 2019 at 3:07 AM Peng Fan <peng.fan@nxp.com> wrote:
> >  
> > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> > > SMC/HVC mailbox
> > >
> > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:  
> > > >
> > > > Hi Jassi,
> > > >  
> > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > > for the ARM SMC/HVC mailbox
> > > > >
> > > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > > >  
> > > > > > > > +examples:
> > > > > > > > +  - |
> > > > > > > > +    sram@910000 {
> > > > > > > > +      compatible = "mmio-sram";
> > > > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > > +      #address-cells = <1>;
> > > > > > > > +      #size-cells = <1>;
> > > > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > > +
> > > > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > +        reg = <0x0 0x200>;
> > > > > > > > +      };
> > > > > > > > +
> > > > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > +        reg = <0x200 0x200>;
> > > > > > > > +      };
> > > > > > > > +    };
> > > > > > > > +
> > > > > > > > +    firmware {
> > > > > > > > +      smc_mbox: mailbox {
> > > > > > > > +        #mbox-cells = <1>;
> > > > > > > > +        compatible = "arm,smc-mbox";
> > > > > > > > +        method = "smc";
> > > > > > > > +        arm,num-chans = <0x2>;
> > > > > > > > +        transports = "mem";
> > > > > > > > +        /* Optional */
> > > > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > > > >  
> > > > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > > > there can only be one instance running platform wide. Right?  
> > > > > >
> > > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > > For virtualization case, there could be dedicated channel for each VM.
> > > > > >  
> > > > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > > > both be active at the same time, right?  
> > > >
> > > > If I get your point correctly,
> > > > On UP, both could not be active. On SMP, tx/rx could be both active,
> > > > anyway this depends on secure firmware and Linux firmware design.
> > > >
> > > > Do you have any suggestions about arm,func-ids here?
> > > >  
> > > I was thinking if this is just an instruction, why can't each channel be
> > > represented as a controller, i.e, have exactly one func-id per controller node.
> > > Define as many controllers as you need channels ?  
> >
> > I am ok, this could make driver code simpler. Something as below?
> >
> >     smc_tx_mbox: tx_mbox {
> >       #mbox-cells = <0>;
> >       compatible = "arm,smc-mbox";
> >       method = "smc";
> >       transports = "mem";
> >       arm,func-id = <0xc20000fe>;
> >     };
> >
> >     smc_rx_mbox: rx_mbox {
> >       #mbox-cells = <0>;
> >       compatible = "arm,smc-mbox";
> >       method = "smc";
> >       transports = "mem";
> >       arm,func-id = <0xc20000ff>;
> >     };
> >
> >     firmware {
> >       scmi {
> >         compatible = "arm,scmi";
> >         mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
> >         mbox-names = "tx", "rx";
> >         shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> >       };
> >     };
> >  
> Yes, the channel part is good.
> But I am not convinced by the need to have SCMI specific "transport" mode.

Why would this be SCMI specific and what is the problem with having this property?
By the very nature of the SMC/HVC call you would expect to also pass parameters in registers. However this limits the amount of data you can push, so the option of reverting to a memory based payload sounds very reasonable.
On the other hand *just* using memory complicates things, in case you have a very simple protocol. You would need a memory region shared between firmware and OS, which is not always easily possible on every platform. Also this doesn't scale easily with multiple mailboxes and channels. Passing parameters via registers is also naturally consistent, as there would be no races and no need for synchronisation with other cores or other users of the mailbox.

So I clearly see the benefit of specifying *both* ways of payload transport. Given that this driver should be protocol agnostic, it makes a lot of sense to introduce both methods *now*, so in the future users can just use the register method, without extending the binding in a incompatible way later (earlier kernels would have the driver, but wouldn't know how to deal with this parameter).

Cheers,
Andre.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andre Przywara]

On Wed, 28 Aug 2019 03:02:58 +0000
Peng Fan <peng.fan@nxp.com> wrote:

Hi,

sorry for the late reply, eventually managed to have a closer look on this.

> 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       | 125 +++++++++++++++++++++
>  1 file changed, 125 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..f8eb28d5e307
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,125 @@
> +# 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. By nature this operation
> +  is synchronous and this mailbox provides no way for asynchronous messages
> +  to be delivered the other way round, from firmware to the OS, but
> +  asynchronous notification could also be supported. However the value of
> +  r0/w0/x0 the firmware returns after the smc call is delivered as a received
> +  message to the mailbox framework, so a synchronous communication can be
> +  established, for a asynchronous notification, no value will be returned.
> +  The exact meaning of both 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:
> +    const: arm,smc-mbox
> +
> +  "#mbox-cells":
> +    const: 1
> +
> +  arm,num-chans:
> +    description: The number of channels supported.
> +    items:
> +      minimum: 1
> +      maximum: 4096 # Should be enough?

This maximum sounds rather arbitrary. Why do we need one? In the driver this just allocates more memory, so why not just impose no artificial limit at all?

Actually, do we need this property at all? Can't we just rely on the size of arm,func-ids to determine this (using of_property_count_elems_of_size() in the driver)? Having both sounds redundant and brings up the question what to do if they don't match.

> +
> +  method:
> +    - enum:
> +        - smc
> +        - hvc
> +
> +  transports:
> +    - enum:
> +        - mem
> +        - reg

Shouldn't there be a description on what both mean, exactly?
For instance I would expect a list of registers to be shown for the "reg" case, and be it by referring to the ARM SMCCC.

Also looking at the driver this brings up more questions:
- Which memory does mem refer to? If this is really the means of transport, it should be referenced in this *controller* node and populated by the driver. Looking at the example below and the driver code, it actually isn't used that way, instead the memory is used and controlled by the mailbox *client*.
- What is the actual difference between the two transports? For "mem" we just populate the registers with 0, for "reg" we use the data. Couldn't this be left to the client?

There are more points which makes me think this property is actually redundant, see my comments on patch 2/2.

> +
> +  arm,func-ids:
> +    description: |
> +      An array of 32-bit values specifying the function IDs used by each
> +      mailbox channel. Those function IDs follow the ARM SMC calling
> +      convention standard [1].
> +
> +      There is one identifier per channel and the number of supported
> +      channels is determined by the length of this array.

I think this makes it obvious that arm,num-chans is not needed.

Also this somewhat contradicts the driver implementation, which allows the array to be shorter, marking this as UINT_MAX and later on using the first data item as a function identifier. This is somewhat surprising and not documented (unless I missed something).

So I would suggest:
- We drop the transports property, and always put the client provided data in the registers, according to the SMCCC. Document this here.
  A client not needing those could always puts zeros (or garbage) in there, the respective firmware would just ignore the registers.
- We drop "arm,num-chans", as this is just redundant with the length of the func-ids array.
- We don't impose an arbitrary limit on the number of channels. From the firmware point of view this is just different function IDs, from Linux' point of view just the size of the memory used. Both don't need to be limited artificially IMHO.
- We mark arm,func-ids as required, as this needs to be fixed, allocated number.

For the question of "always one channel per controller" vs. "allow multiple channels per controller": I don't really have a strong opinion, but lean towards allowing multiple channels. This would allow to group functions belonging together, separating them from totally distinct controller uses (think virtual GPIO vs. SCMI).
And it would still allow the special case of multiple single-channel controllers to be naturally specified.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 0
> +    maxItems: 4096   # Should be enough?
> +
> +required:
> +  - compatible
> +  - "#mbox-cells"
> +  - arm,num-chans
> +  - transports
> +  - method

According to the above description arm,func-ids would also be required?

Cheers,
Andre.

> +
> +examples:
> +  - |
> +    sram@910000 {
> +      compatible = "mmio-sram";
> +      reg = <0x0 0x93f000 0x0 0x1000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      ranges = <0 0x0 0x93f000 0x1000>;
> +
> +      cpu_scp_lpri: scp-shmem@0 {
> +        compatible = "arm,scmi-shmem";
> +        reg = <0x0 0x200>;
> +      };
> +
> +      cpu_scp_hpri: scp-shmem@200 {
> +        compatible = "arm,scmi-shmem";
> +        reg = <0x200 0x200>;
> +      };
> +    };
> +
> +    firmware {
> +      smc_mbox: mailbox {
> +        #mbox-cells = <1>;
> +        compatible = "arm,smc-mbox";
> +        method = "smc";
> +        arm,num-chans = <0x2>;
> +        transports = "mem";
> +        /* Optional */
> +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> +      };
> +
> +      scmi {
> +        compatible = "arm,scmi";
> +        mboxes = <&smc_mbox 0>, <&smc_mbox 1>;
> +        mbox-names = "tx", "rx";
> +        shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> +      };
> +    };
> +
> +...


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andre Przywara]

On Wed, 28 Aug 2019 03:03:02 +0000
Peng Fan <peng.fan@nxp.com> wrote:

Hi,

> 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 | 215 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 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..76a2ae11ee4d
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,215 @@
> +// 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>
> +
> +#define ARM_SMC_MBOX_MEM_TRANS	BIT(0)
> +
> +struct arm_smc_chan_data {
> +	u32 function_id;
> +	u32 chan_id;
> +	u32 flags;
> +};
> +
> +struct arm_smccc_mbox_cmd {
> +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7;

I think this is one or even two registers too long?
The SMCCC speaks of the function ID in x0/r0 and six arguments, with a "client ID" being an optional seventh argument. Looking at the description there I believe we cannot use the client ID here for this purpose, as this is supposed to be set by a hypervisor before passing on an SMC to EL3 firmware. KVM does not allow passing on SMCs in this way.

Also, while using "long" in here seems to make sense from the mailbox and SMC point of view, aliasing this to the mailbox client provided data seems dangerous to me, as this exposes the difference between arm32 and arm64 to the client. I think this is not what we want, the client should not be architecture specific.

> +};
> +
> +typedef unsigned long (smc_mbox_fn)(unsigned long, unsigned long,
> +				    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;
> +	u32 chan_id;
> +
> +	if (chan_data->flags & ARM_SMC_MBOX_MEM_TRANS) {
> +		if (chan_data->function_id != UINT_MAX)
> +			function_id = chan_data->function_id;
> +		else
> +			function_id = cmd->a0;

This is somewhat surprising, dangerous and undocumented. We should *not* allow mailbox clients to specify the function ID. They could end up using PSCI function IDs, for instance, that sounds especially scary if a client driver allows userland to set parameters of some sort.
The function ID is presumably allocated and fixed in the firmware, so it should not be dynamic. Any dynamic properties should be done within a function ID on the protocol level, by using r1/x1, for instance.

> +		chan_id = chan_data->chan_id;

Why is this here? Where is this documented? Isn't this redundant with function ID? Or is this meant to be a replacement for it when a client provided function ID is used (which is not desired, as mentioned above)?

> +		ret = invoke_smc_mbox_fn(function_id, chan_id, 0, 0, 0, 0,
> +					 0, 0);
> +	} else {
> +		ret = invoke_smc_mbox_fn(cmd->a0, cmd->a1, cmd->a2, cmd->a3,
> +					 cmd->a4, cmd->a5, cmd->a6, cmd->a7);
> +	}

As mentioned in my reply to the binding patch, I don't see this is necessary. Instead of ignoring the client provided data, we should just always pass it on. If clients and protocols don't use them, the client could zero it as well, letting the firmware side ignore it.

Also this underlines the problem with using "long" above: For 32-bit and 64-bit kernels the layout would be different.
I think the size of each argument should be determined by the calling convention class (bit 30) of the function ID.
The client doesn't know about that one (it's a controller/firmware property), so this driver here should split up the stream of data according to SMC64 vs. SMC32.
Does that make sense?

> +
> +	mbox_chan_received_data(link, (void *)ret);

Not a mailbox expert, but shouldn't we mark the TX operation as complete here? Clearly by returning from the SMC the firmware has received the request.
Whether the requested action has completed, is a protocol / mailbox client question.

> +
> +	return 0;
> +}
> +
> +static unsigned long __invoke_fn_hvc(unsigned long function_id,
> +				     unsigned long arg0, unsigned long arg1,
> +				     unsigned long arg2, unsigned long arg3,
> +				     unsigned long arg4, unsigned long arg5,
> +				     unsigned long arg6)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
> +		      arg5, arg6, &res);
> +	return res.a0;
> +}
> +
> +static unsigned long __invoke_fn_smc(unsigned long function_id,
> +				     unsigned long arg0, unsigned long arg1,
> +				     unsigned long arg2, unsigned long arg3,
> +				     unsigned long arg4, unsigned long arg5,
> +				     unsigned long arg6)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
> +		      arg5, arg6, &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;
> +	const char *method;
> +	bool mem_trans = false;
> +	int ret, i;
> +	u32 val;
> +
> +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> +		if (!val) {
> +			dev_err(dev, "invalid arm,num-chans value %u\n", val);
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}

As mentioned, this property should be removed, ...

> +
> +	if (!of_property_read_string(dev->of_node, "transports", &method)) {
> +		if (!strcmp("mem", method)) {
> +			mem_trans = true;
> +		} else if (!strcmp("reg", method)) {
> +			mem_trans = false;
> +		} else {
> +			dev_warn(dev, "invalid \"transports\" property: %s\n",
> +				 method);
> +
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}

... and this one as well.

> +
> +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> +		if (!strcmp("hvc", method)) {
> +			invoke_smc_mbox_fn = __invoke_fn_hvc;
> +		} else if (!strcmp("smc", method)) {
> +			invoke_smc_mbox_fn = __invoke_fn_smc;
> +		} else {
> +			dev_warn(dev, "invalid \"method\" property: %s\n",
> +				 method);

Just a nit, but if this is fatal for the driver, it should be dev_err().

> +
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (!mbox)
> +		return -ENOMEM;
> +
> +	mbox->num_chans = val;

This could be replaced with:
	mbox->num_chans = of_property_count_elems_of_size(dev_of_node(dev),
                                     "arm,func-ids", sizeof(u32));

> +	mbox->chans = devm_kcalloc(dev, mbox->num_chans, sizeof(*mbox->chans),
> +				   GFP_KERNEL);
> +	if (!mbox->chans)
> +		return -ENOMEM;
> +
> +	chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
> +				 GFP_KERNEL);
> +	if (!chan_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < mbox->num_chans; i++) {
> +		u32 function_id;
> +
> +		ret = of_property_read_u32_index(dev->of_node,
> +						 "arm,func-ids", i,
> +						 &function_id);
> +		if (ret)
> +			chan_data[i].function_id = UINT_MAX;
> +
> +		else
> +			chan_data[i].function_id = function_id;

As mentioned above, this should go. Any non-0 return value should stop the driver probing.

Cheers,
Andre.

> +
> +		chan_data[i].chan_id = i;
> +
> +		if (mem_trans)
> +			chan_data[i].flags |= ARM_SMC_MBOX_MEM_TRANS;
> +		mbox->chans[i].con_priv = &chan_data[i];
> +	}
> +
> +	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 with %d chan%s.\n",
> +		 mbox->num_chans, mbox->num_chans == 1 ? "" : "s");
> +
> +	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", },
> +	{},
> +};
> +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("Andre Przywara <andre.przywara@arm.com>");
> +MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
> +MODULE_LICENSE("GPL v2");


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Fri, 30 Aug 2019 03:12:29 -0500
> Jassi Brar <jassisinghbrar@gmail.com> wrote:
> 
> Hi,
> 
> > On Fri, Aug 30, 2019 at 3:07 AM Peng Fan <peng.fan@nxp.com> wrote:
> > >
> > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > for the ARM SMC/HVC mailbox
> > > >
> > > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > > >
> > > > > Hi Jassi,
> > > > >
> > > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding
> > > > > > doc for the ARM SMC/HVC mailbox
> > > > > >
> > > > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com>
> wrote:
> > > > > >
> > > > > > > > > +examples:
> > > > > > > > > +  - |
> > > > > > > > > +    sram@910000 {
> > > > > > > > > +      compatible = "mmio-sram";
> > > > > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > > > +      #address-cells = <1>;
> > > > > > > > > +      #size-cells = <1>;
> > > > > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > > > +
> > > > > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > +        reg = <0x0 0x200>;
> > > > > > > > > +      };
> > > > > > > > > +
> > > > > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > +        reg = <0x200 0x200>;
> > > > > > > > > +      };
> > > > > > > > > +    };
> > > > > > > > > +
> > > > > > > > > +    firmware {
> > > > > > > > > +      smc_mbox: mailbox {
> > > > > > > > > +        #mbox-cells = <1>;
> > > > > > > > > +        compatible = "arm,smc-mbox";
> > > > > > > > > +        method = "smc";
> > > > > > > > > +        arm,num-chans = <0x2>;
> > > > > > > > > +        transports = "mem";
> > > > > > > > > +        /* Optional */
> > > > > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > > > > >
> > > > > > > > SMC/HVC is synchronously(block) running in "secure mode",
> > > > > > > > i.e, there can only be one instance running platform wide. Right?
> > > > > > >
> > > > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > > > For virtualization case, there could be dedicated channel for each
> VM.
> > > > > > >
> > > > > > I am talking from Linux pov. Functions 0xfe and 0xff above,
> > > > > > can't both be active at the same time, right?
> > > > >
> > > > > If I get your point correctly,
> > > > > On UP, both could not be active. On SMP, tx/rx could be both
> > > > > active, anyway this depends on secure firmware and Linux firmware
> design.
> > > > >
> > > > > Do you have any suggestions about arm,func-ids here?
> > > > >
> > > > I was thinking if this is just an instruction, why can't each
> > > > channel be represented as a controller, i.e, have exactly one func-id per
> controller node.
> > > > Define as many controllers as you need channels ?
> > >
> > > I am ok, this could make driver code simpler. Something as below?
> > >
> > >     smc_tx_mbox: tx_mbox {
> > >       #mbox-cells = <0>;
> > >       compatible = "arm,smc-mbox";
> > >       method = "smc";
> > >       transports = "mem";
> > >       arm,func-id = <0xc20000fe>;
> > >     };
> > >
> > >     smc_rx_mbox: rx_mbox {
> > >       #mbox-cells = <0>;
> > >       compatible = "arm,smc-mbox";
> > >       method = "smc";
> > >       transports = "mem";
> > >       arm,func-id = <0xc20000ff>;
> > >     };
> > >
> > >     firmware {
> > >       scmi {
> > >         compatible = "arm,scmi";
> > >         mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
> > >         mbox-names = "tx", "rx";
> > >         shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> > >       };
> > >     };
> > >
> > Yes, the channel part is good.
> > But I am not convinced by the need to have SCMI specific "transport" mode.
> 
> Why would this be SCMI specific and what is the problem with having this
> property?
> By the very nature of the SMC/HVC call you would expect to also pass
> parameters in registers. However this limits the amount of data you can push,
> so the option of reverting to a memory based payload sounds very
> reasonable.
> On the other hand *just* using memory complicates things, in case you have a
> very simple protocol. You would need a memory region shared between
> firmware and OS, which is not always easily possible on every platform. Also
> this doesn't scale easily with multiple mailboxes and channels. Passing
> parameters via registers is also naturally consistent, as there would be no
> races and no need for synchronisation with other cores or other users of the
> mailbox.
> 
> So I clearly see the benefit of specifying *both* ways of payload transport.
> Given that this driver should be protocol agnostic, it makes a lot of sense to
> introduce both methods *now*, so in the future users can just use the register
> method, without extending the binding in a incompatible way later (earlier
> kernels would have the driver, but wouldn't know how to deal with this
> parameter).

Andre, thanks for your explanation.
Jassi, are you ok that this property "transport" is kept in V6?

Thanks,
Peng.
> 
> Cheers,
> Andre.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Mon, Sep 9, 2019 at 8:32 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Fri, 30 Aug 2019 03:12:29 -0500
> Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> Hi,
>
> > On Fri, Aug 30, 2019 at 3:07 AM Peng Fan <peng.fan@nxp.com> wrote:
> > >
> > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> > > > SMC/HVC mailbox
> > > >
> > > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > > >
> > > > > Hi Jassi,
> > > > >
> > > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > > > for the ARM SMC/HVC mailbox
> > > > > >
> > > > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > >
> > > > > > > > > +examples:
> > > > > > > > > +  - |
> > > > > > > > > +    sram@910000 {
> > > > > > > > > +      compatible = "mmio-sram";
> > > > > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > > > +      #address-cells = <1>;
> > > > > > > > > +      #size-cells = <1>;
> > > > > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > > > +
> > > > > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > +        reg = <0x0 0x200>;
> > > > > > > > > +      };
> > > > > > > > > +
> > > > > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > +        reg = <0x200 0x200>;
> > > > > > > > > +      };
> > > > > > > > > +    };
> > > > > > > > > +
> > > > > > > > > +    firmware {
> > > > > > > > > +      smc_mbox: mailbox {
> > > > > > > > > +        #mbox-cells = <1>;
> > > > > > > > > +        compatible = "arm,smc-mbox";
> > > > > > > > > +        method = "smc";
> > > > > > > > > +        arm,num-chans = <0x2>;
> > > > > > > > > +        transports = "mem";
> > > > > > > > > +        /* Optional */
> > > > > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > > > > >
> > > > > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > > > > there can only be one instance running platform wide. Right?
> > > > > > >
> > > > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > > > For virtualization case, there could be dedicated channel for each VM.
> > > > > > >
> > > > > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > > > > both be active at the same time, right?
> > > > >
> > > > > If I get your point correctly,
> > > > > On UP, both could not be active. On SMP, tx/rx could be both active,
> > > > > anyway this depends on secure firmware and Linux firmware design.
> > > > >
> > > > > Do you have any suggestions about arm,func-ids here?
> > > > >
> > > > I was thinking if this is just an instruction, why can't each channel be
> > > > represented as a controller, i.e, have exactly one func-id per controller node.
> > > > Define as many controllers as you need channels ?
> > >
> > > I am ok, this could make driver code simpler. Something as below?
> > >
> > >     smc_tx_mbox: tx_mbox {
> > >       #mbox-cells = <0>;
> > >       compatible = "arm,smc-mbox";
> > >       method = "smc";
> > >       transports = "mem";
> > >       arm,func-id = <0xc20000fe>;
> > >     };
> > >
> > >     smc_rx_mbox: rx_mbox {
> > >       #mbox-cells = <0>;
> > >       compatible = "arm,smc-mbox";
> > >       method = "smc";
> > >       transports = "mem";
> > >       arm,func-id = <0xc20000ff>;
> > >     };
> > >
> > >     firmware {
> > >       scmi {
> > >         compatible = "arm,scmi";
> > >         mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
> > >         mbox-names = "tx", "rx";
> > >         shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> > >       };
> > >     };
> > >
> > Yes, the channel part is good.
> > But I am not convinced by the need to have SCMI specific "transport" mode.
>
> Why would this be SCMI specific and what is the problem with having this property?
> By the very nature of the SMC/HVC call you would expect to also pass parameters in registers.
> However this limits the amount of data you can push, so the option of reverting to a
> memory based payload sounds very reasonable.
>
Of course, it is very legit to pass data via mem and many platforms do
that. But as you note in your next post, the 'transport' doesn't seem
necessary doing what it does in the driver.

Cheers!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Mon, Sep 9, 2019 at 10:42 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Wed, 28 Aug 2019 03:02:58 +0000
> Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi,
>
> sorry for the late reply, eventually managed to have a closer look on this.
>
> > 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       | 125 +++++++++++++++++++++
> >  1 file changed, 125 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..f8eb28d5e307
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,125 @@
> > +# 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. By nature this operation
> > +  is synchronous and this mailbox provides no way for asynchronous messages
> > +  to be delivered the other way round, from firmware to the OS, but
> > +  asynchronous notification could also be supported. However the value of
> > +  r0/w0/x0 the firmware returns after the smc call is delivered as a received
> > +  message to the mailbox framework, so a synchronous communication can be
> > +  established, for a asynchronous notification, no value will be returned.
> > +  The exact meaning of both 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:
> > +    const: arm,smc-mbox
> > +
> > +  "#mbox-cells":
> > +    const: 1
> > +
> > +  arm,num-chans:
> > +    description: The number of channels supported.
> > +    items:
> > +      minimum: 1
> > +      maximum: 4096 # Should be enough?
>
> This maximum sounds rather arbitrary. Why do we need one? In the driver this just allocates more memory, so why not just impose no artificial limit at all?
>
This will be gone, once the driver is converted to 1channel per controller.

> Actually, do we need this property at all? Can't we just rely on the size of arm,func-ids to determine this (using of_property_count_elems_of_size() in the driver)? Having both sounds redundant and brings up the question what to do if they don't match.
>

> > +
> > +  method:
> > +    - enum:
> > +        - smc
> > +        - hvc
> > +
> > +  transports:
> > +    - enum:
> > +        - mem
> > +        - reg
>
> Shouldn't there be a description on what both mean, exactly?
> For instance I would expect a list of registers to be shown for the "reg" case, and be it by referring to the ARM SMCCC.
>
> Also looking at the driver this brings up more questions:
> - Which memory does mem refer to? If this is really the means of transport, it should be referenced in this *controller* node and populated by the driver. Looking at the example below and the driver code, it actually isn't used that way, instead the memory is used and controlled by the mailbox *client*.
> - What is the actual difference between the two transports? For "mem" we just populate the registers with 0, for "reg" we use the data. Couldn't this be left to the client?
>
> There are more points which makes me think this property is actually redundant, see my comments on patch 2/2.
>
> > +
> > +  arm,func-ids:
> > +    description: |
> > +      An array of 32-bit values specifying the function IDs used by each
> > +      mailbox channel. Those function IDs follow the ARM SMC calling
> > +      convention standard [1].
> > +
> > +      There is one identifier per channel and the number of supported
> > +      channels is determined by the length of this array.
>
> I think this makes it obvious that arm,num-chans is not needed.
>
> Also this somewhat contradicts the driver implementation, which allows the array to be shorter, marking this as UINT_MAX and later on using the first data item as a function identifier. This is somewhat surprising and not documented (unless I missed something).
>
> So I would suggest:
> - We drop the transports property, and always put the client provided data in the registers, according to the SMCCC. Document this here.
>   A client not needing those could always puts zeros (or garbage) in there, the respective firmware would just ignore the registers.
> - We drop "arm,num-chans", as this is just redundant with the length of the func-ids array.
> - We don't impose an arbitrary limit on the number of channels. From the firmware point of view this is just different function IDs, from Linux' point of view just the size of the memory used. Both don't need to be limited artificially IMHO.
>
Sounds like we are in sync.

> - We mark arm,func-ids as required, as this needs to be fixed, allocated number.
>
I still think func-id can be done without. A client can always pass
the value as it knows what it expects. But I can live with it being
optional.

cheers!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Andre,

> Subject: Re: [PATCH v5 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Wed, 28 Aug 2019 03:03:02 +0000
> Peng Fan <peng.fan@nxp.com> wrote:
> 
> Hi,
> 
> > 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%7Cce8d04bcbdea4de6a77a08d7353c4e35%7C686ea1d3bc2b
> 4c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C637036405476727893&amp;sdata=y5%2FI%2B
> w%2FPOypEh
> > zh6gWdXAGMGnl677B7gDZsX%2F5zfAQw%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 | 215
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 224 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..76a2ae11ee4d
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,215 @@
> > +// 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>
> > +
> > +#define ARM_SMC_MBOX_MEM_TRANS	BIT(0)
> > +
> > +struct arm_smc_chan_data {
> > +	u32 function_id;
> > +	u32 chan_id;
> > +	u32 flags;
> > +};
> > +
> > +struct arm_smccc_mbox_cmd {
> > +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
> 
> I think this is one or even two registers too long?
> The SMCCC speaks of the function ID in x0/r0 and six arguments, with a
> "client ID" being an optional seventh argument. Looking at the description
> there I believe we cannot use the client ID here for this purpose, as this is
> supposed to be set by a hypervisor before passing on an SMC to EL3 firmware.
> KVM does not allow passing on SMCs in this way.

I'll drop a7.

> 
> Also, while using "long" in here seems to make sense from the mailbox and
> SMC point of view, aliasing this to the mailbox client provided data seems
> dangerous to me, as this exposes the difference between arm32 and arm64
> to the client. I think this is not what we want, the client should not be
> architecture specific.

I see.

> 
> > +};
> > +
> > +typedef unsigned long (smc_mbox_fn)(unsigned long, unsigned long,
> > +				    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;
> > +	u32 chan_id;
> > +
> > +	if (chan_data->flags & ARM_SMC_MBOX_MEM_TRANS) {
> > +		if (chan_data->function_id != UINT_MAX)
> > +			function_id = chan_data->function_id;
> > +		else
> > +			function_id = cmd->a0;
> 
> This is somewhat surprising, dangerous and undocumented. We should *not*
> allow mailbox clients to specify the function ID. They could end up using PSCI
> function IDs, for instance, that sounds especially scary if a client driver allows
> userland to set parameters of some sort.

So the func id should not be an optional property in dts?
The check here is to not make func id an optional property and allow client driver
pass function id.

> The function ID is presumably allocated and fixed in the firmware, so it should
> not be dynamic. Any dynamic properties should be done within a function ID
> on the protocol level, by using r1/x1, for instance.
> 
> > +		chan_id = chan_data->chan_id;
> 
> Why is this here? Where is this documented? Isn't this redundant with
> function ID? Or is this meant to be a replacement for it when a client provided
> function ID is used (which is not desired, as mentioned above)?

This will be dropped after move to per channel as a controller.

> 
> > +		ret = invoke_smc_mbox_fn(function_id, chan_id, 0, 0, 0, 0,
> > +					 0, 0);
> > +	} else {
> > +		ret = invoke_smc_mbox_fn(cmd->a0, cmd->a1, cmd->a2, cmd->a3,
> > +					 cmd->a4, cmd->a5, cmd->a6, cmd->a7);
> > +	}
> 
> As mentioned in my reply to the binding patch, I don't see this is necessary.
> Instead of ignoring the client provided data, we should just always pass it on.
> If clients and protocols don't use them, the client could zero it as well, letting
> the firmware side ignore it.

Per Jassi's comments, arm,func-id is an optional property.

So I would change to like below:
+       if (func_id)
+               function_id = chan_data->function_id;
+       else
+               function_id = cmd->a0;
+
+       if (data)
+               ret = invoke_smc_mbox_fn(function_id, cmd->args[1], cmd->args[2], cmd->args[3], cmd->args[4], cmd->args[5], cmd->args[6]);
+       else
+               ret = invoke_smc_mbox_fn(function_id, 0, 0, 0, 0, 0, 0);

And need a check if no data from client and no func_id, need return -EINVAL to client.

How do you think?

> 
> Also this underlines the problem with using "long" above: For 32-bit and 64-bit
> kernels the layout would be different.
> I think the size of each argument should be determined by the calling
> convention class (bit 30) of the function ID.
> The client doesn't know about that one (it's a controller/firmware property),
> so this driver here should split up the stream of data according to SMC64 vs.
> SMC32.
> Does that make sense?

Got it. Check the bit30 to decide use r[] or use x[] for SMCCC.

> 
> > +
> > +	mbox_chan_received_data(link, (void *)ret);
> 
> Not a mailbox expert, but shouldn't we mark the TX operation as complete
> here? Clearly by returning from the SMC the firmware has received the
> request.
> Whether the requested action has completed, is a protocol / mailbox client
> question.

I do not see any API to mark TX completed. My understanding is send_data
return value means complete finished.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned long __invoke_fn_hvc(unsigned long function_id,
> > +				     unsigned long arg0, unsigned long arg1,
> > +				     unsigned long arg2, unsigned long arg3,
> > +				     unsigned long arg4, unsigned long arg5,
> > +				     unsigned long arg6)
> > +{
> > +	struct arm_smccc_res res;
> > +
> > +	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
> > +		      arg5, arg6, &res);
> > +	return res.a0;
> > +}
> > +
> > +static unsigned long __invoke_fn_smc(unsigned long function_id,
> > +				     unsigned long arg0, unsigned long arg1,
> > +				     unsigned long arg2, unsigned long arg3,
> > +				     unsigned long arg4, unsigned long arg5,
> > +				     unsigned long arg6)
> > +{
> > +	struct arm_smccc_res res;
> > +
> > +	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
> > +		      arg5, arg6, &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;
> > +	const char *method;
> > +	bool mem_trans = false;
> > +	int ret, i;
> > +	u32 val;
> > +
> > +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> > +		if (!val) {
> > +			dev_err(dev, "invalid arm,num-chans value %u\n", val);
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> 
> As mentioned, this property should be removed, ...

Yes.

> 
> > +
> > +	if (!of_property_read_string(dev->of_node, "transports", &method)) {
> > +		if (!strcmp("mem", method)) {
> > +			mem_trans = true;
> > +		} else if (!strcmp("reg", method)) {
> > +			mem_trans = false;
> > +		} else {
> > +			dev_warn(dev, "invalid \"transports\" property: %s\n",
> > +				 method);
> > +
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> 
> ... and this one as well.

Yes.

> 
> > +
> > +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> > +		if (!strcmp("hvc", method)) {
> > +			invoke_smc_mbox_fn = __invoke_fn_hvc;
> > +		} else if (!strcmp("smc", method)) {
> > +			invoke_smc_mbox_fn = __invoke_fn_smc;
> > +		} else {
> > +			dev_warn(dev, "invalid \"method\" property: %s\n",
> > +				 method);
> 
> Just a nit, but if this is fatal for the driver, it should be dev_err().

Yes.

> 
> > +
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > +	if (!mbox)
> > +		return -ENOMEM;
> > +
> > +	mbox->num_chans = val;
> 
> This could be replaced with:
> 	mbox->num_chans =
> of_property_count_elems_of_size(dev_of_node(dev),
>                                      "arm,func-ids", sizeof(u32));
> 
> > +	mbox->chans = devm_kcalloc(dev, mbox->num_chans,
> sizeof(*mbox->chans),
> > +				   GFP_KERNEL);
> > +	if (!mbox->chans)
> > +		return -ENOMEM;
> > +
> > +	chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
> > +				 GFP_KERNEL);
> > +	if (!chan_data)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < mbox->num_chans; i++) {
> > +		u32 function_id;
> > +
> > +		ret = of_property_read_u32_index(dev->of_node,
> > +						 "arm,func-ids", i,
> > +						 &function_id);
> > +		if (ret)
> > +			chan_data[i].function_id = UINT_MAX;
> > +
> > +		else
> > +			chan_data[i].function_id = function_id;
> 
> As mentioned above, this should go. Any non-0 return value should stop the
> driver probing.

But if the func-id as an optional property, we need use the value from client
driver.

Thanks,
Peng.
> 
> Cheers,
> Andre.
> 
> > +
> > +		chan_data[i].chan_id = i;
> > +
> > +		if (mem_trans)
> > +			chan_data[i].flags |= ARM_SMC_MBOX_MEM_TRANS;
> > +		mbox->chans[i].con_priv = &chan_data[i];
> > +	}
> > +
> > +	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 with %d chan%s.\n",
> > +		 mbox->num_chans, mbox->num_chans == 1 ? "" : "s");
> > +
> > +	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", },
> > +	{},
> > +};
> > +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("Andre Przywara <andre.przywara@arm.com>");
> > +MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
> > +MODULE_LICENSE("GPL v2");


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andre Przywara]

On Tue, 10 Sep 2019 21:36:35 -0500
Jassi Brar <jassisinghbrar@gmail.com> wrote:

Hi,

> On Mon, Sep 9, 2019 at 8:32 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Fri, 30 Aug 2019 03:12:29 -0500
> > Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > Hi,
> >  
> > > On Fri, Aug 30, 2019 at 3:07 AM Peng Fan <peng.fan@nxp.com> wrote:  
> > > >  
> > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> > > > > SMC/HVC mailbox
> > > > >
> > > > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@nxp.com> wrote:  
> > > > > >
> > > > > > Hi Jassi,
> > > > > >  
> > > > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > > > > for the ARM SMC/HVC mailbox
> > > > > > >
> > > > > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > >  
> > > > > > > > > > +examples:
> > > > > > > > > > +  - |
> > > > > > > > > > +    sram@910000 {
> > > > > > > > > > +      compatible = "mmio-sram";
> > > > > > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > > > > +      #address-cells = <1>;
> > > > > > > > > > +      #size-cells = <1>;
> > > > > > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > > > > +
> > > > > > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > > +        reg = <0x0 0x200>;
> > > > > > > > > > +      };
> > > > > > > > > > +
> > > > > > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > > +        reg = <0x200 0x200>;
> > > > > > > > > > +      };
> > > > > > > > > > +    };
> > > > > > > > > > +
> > > > > > > > > > +    firmware {
> > > > > > > > > > +      smc_mbox: mailbox {
> > > > > > > > > > +        #mbox-cells = <1>;
> > > > > > > > > > +        compatible = "arm,smc-mbox";
> > > > > > > > > > +        method = "smc";
> > > > > > > > > > +        arm,num-chans = <0x2>;
> > > > > > > > > > +        transports = "mem";
> > > > > > > > > > +        /* Optional */
> > > > > > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > > > > > >  
> > > > > > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > > > > > there can only be one instance running platform wide. Right?  
> > > > > > > >
> > > > > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > > > > For virtualization case, there could be dedicated channel for each VM.
> > > > > > > >  
> > > > > > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > > > > > both be active at the same time, right?  
> > > > > >
> > > > > > If I get your point correctly,
> > > > > > On UP, both could not be active. On SMP, tx/rx could be both active,
> > > > > > anyway this depends on secure firmware and Linux firmware design.
> > > > > >
> > > > > > Do you have any suggestions about arm,func-ids here?
> > > > > >  
> > > > > I was thinking if this is just an instruction, why can't each channel be
> > > > > represented as a controller, i.e, have exactly one func-id per controller node.
> > > > > Define as many controllers as you need channels ?  
> > > >
> > > > I am ok, this could make driver code simpler. Something as below?
> > > >
> > > >     smc_tx_mbox: tx_mbox {
> > > >       #mbox-cells = <0>;
> > > >       compatible = "arm,smc-mbox";
> > > >       method = "smc";
> > > >       transports = "mem";
> > > >       arm,func-id = <0xc20000fe>;
> > > >     };
> > > >
> > > >     smc_rx_mbox: rx_mbox {
> > > >       #mbox-cells = <0>;
> > > >       compatible = "arm,smc-mbox";
> > > >       method = "smc";
> > > >       transports = "mem";
> > > >       arm,func-id = <0xc20000ff>;
> > > >     };
> > > >
> > > >     firmware {
> > > >       scmi {
> > > >         compatible = "arm,scmi";
> > > >         mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
> > > >         mbox-names = "tx", "rx";
> > > >         shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> > > >       };
> > > >     };
> > > >  
> > > Yes, the channel part is good.
> > > But I am not convinced by the need to have SCMI specific "transport" mode.  
> >
> > Why would this be SCMI specific and what is the problem with having this property?
> > By the very nature of the SMC/HVC call you would expect to also pass parameters in registers.
> > However this limits the amount of data you can push, so the option of reverting to a
> > memory based payload sounds very reasonable.
> >  
> Of course, it is very legit to pass data via mem and many platforms do
> that. But as you note in your next post, the 'transport' doesn't seem
> necessary doing what it does in the driver.

Yes, indeed. I didn't realise that until looking more deeply into the driver later.
So I think we are on the same page regarding this: the *controller* driver and its binding does not need to know about the transport, that's something between the mailbox client and the firmware implementation.

Cheers,
Andre.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andre Przywara]

On Tue, 10 Sep 2019 21:44:11 -0500
Jassi Brar <jassisinghbrar@gmail.com> wrote:

Hi,

> On Mon, Sep 9, 2019 at 10:42 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Wed, 28 Aug 2019 03:02:58 +0000
> > Peng Fan <peng.fan@nxp.com> wrote:
> >
[ ... ]
> >  
> > > +
> > > +  arm,func-ids:
> > > +    description: |
> > > +      An array of 32-bit values specifying the function IDs used by each
> > > +      mailbox channel. Those function IDs follow the ARM SMC calling
> > > +      convention standard [1].
> > > +
> > > +      There is one identifier per channel and the number of supported
> > > +      channels is determined by the length of this array.  
> >
> > I think this makes it obvious that arm,num-chans is not needed.
> >
> > Also this somewhat contradicts the driver implementation, which allows the array to be shorter, marking this as UINT_MAX and later on using the first data item as a function identifier. This is somewhat surprising and not documented (unless I missed something).
> >
> > So I would suggest:
> > - We drop the transports property, and always put the client provided data in the registers, according to the SMCCC. Document this here.
> >   A client not needing those could always puts zeros (or garbage) in there, the respective firmware would just ignore the registers.
> > - We drop "arm,num-chans", as this is just redundant with the length of the func-ids array.
> > - We don't impose an arbitrary limit on the number of channels. From the firmware point of view this is just different function IDs, from Linux' point of view just the size of the memory used. Both don't need to be limited artificially IMHO.
> >  
> Sounds like we are in sync.
> 
> > - We mark arm,func-ids as required, as this needs to be fixed, allocated number.
> >  
> I still think func-id can be done without. A client can always pass
> the value as it knows what it expects.

I don't think it's the right abstraction. The mailbox *controller* uses a specific func-id, this has to match the one the firmware expects. So this is a property of the mailbox transport channel (the SMC call), and the *client* should *not* care about it. It just sees the logical channel ID (if we have one), which the controller translates into the func-ID.

So it should really look like this (assuming only single channel controllers):
mailbox: smc-mailbox {
    #mbox-cells = <0>;
    compatible = "arm,smc-mbox";
    method = "smc";
    arm,func-id = <0x820000fe>;
};
scmi {
    compatible = "arm,scmi";
    mboxes = <&smc_mbox>;
    mbox-names = "tx"; /* rx is optional */
    shmem = <&cpu_scp_hpri>;
};

If you allow the client to provide the function ID (and I am not saying this is a good idea): where would this func ID come from? It would need to be a property of the client DT node, then. So one way would be to use the func ID as the Linux mailbox channel ID:
mailbox: smc-mailbox {
    #mbox-cells = <1>;
    compatible = "arm,smc-mbox";
    method = "smc";
};
scmi {
    compatible = "arm,scmi";
    mboxes = <&smc_mbox 0x820000fe>;
    mbox-names = "tx"; /* rx is optional */
    shmem = <&cpu_scp_hpri>;
};

But this doesn't look desirable.

And as I mentioned this before: allowing some mailbox clients to provide the function IDs sound scary, as they could use anything they want, triggering random firmware actions (think PSCI_CPU_OFF).

So I think we should have a required "arm,func-id" property, with exactly one 32-bit value (again assuming single channel controllers).

Cheers,
Andre.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Wed, Sep 11, 2019 at 10:03 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Tue, 10 Sep 2019 21:44:11 -0500
> Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> Hi,
>
> > On Mon, Sep 9, 2019 at 10:42 AM Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > On Wed, 28 Aug 2019 03:02:58 +0000
> > > Peng Fan <peng.fan@nxp.com> wrote:
> > >
> [ ... ]
> > >
> > > > +
> > > > +  arm,func-ids:
> > > > +    description: |
> > > > +      An array of 32-bit values specifying the function IDs used by each
> > > > +      mailbox channel. Those function IDs follow the ARM SMC calling
> > > > +      convention standard [1].
> > > > +
> > > > +      There is one identifier per channel and the number of supported
> > > > +      channels is determined by the length of this array.
> > >
> > > I think this makes it obvious that arm,num-chans is not needed.
> > >
> > > Also this somewhat contradicts the driver implementation, which allows the array to be shorter, marking this as UINT_MAX and later on using the first data item as a function identifier. This is somewhat surprising and not documented (unless I missed something).
> > >
> > > So I would suggest:
> > > - We drop the transports property, and always put the client provided data in the registers, according to the SMCCC. Document this here.
> > >   A client not needing those could always puts zeros (or garbage) in there, the respective firmware would just ignore the registers.
> > > - We drop "arm,num-chans", as this is just redundant with the length of the func-ids array.
> > > - We don't impose an arbitrary limit on the number of channels. From the firmware point of view this is just different function IDs, from Linux' point of view just the size of the memory used. Both don't need to be limited artificially IMHO.
> > >
> > Sounds like we are in sync.
> >
> > > - We mark arm,func-ids as required, as this needs to be fixed, allocated number.
> > >
> > I still think func-id can be done without. A client can always pass
> > the value as it knows what it expects.
>
> I don't think it's the right abstraction. The mailbox *controller* uses a specific func-id, this has to match the one the firmware expects. So this is a property of the mailbox transport channel (the SMC call), and the *client* should *not* care about it. It just sees the logical channel ID (if we have one), which the controller translates into the func-ID.
>
arg0 is special only to the client/protocol, otherwise it is simply
the first argument for the arm_smccc_smc *instruction* controller.
arg[1,7] are already provided by the client, so it is only neater if
arg0 is also taken from the client.

But as I said, I am still ok if func-id is passed from dt and arg0
from client is ignored because we have one channel per controller
design and we don't have to worry about number of channels there can
be dedicated to specific functions.

> So it should really look like this (assuming only single channel controllers):
> mailbox: smc-mailbox {
>     #mbox-cells = <0>;
>     compatible = "arm,smc-mbox";
>     method = "smc";
>
Do we want to do away with 'method' property and use different
'compatible' properties instead?
 compatible = "arm,smc-mbox";     or    compatible = "arm,hvc-mbox";

>     arm,func-id = <0x820000fe>;
> };
> scmi {
>     compatible = "arm,scmi";
>     mboxes = <&smc_mbox>;
>     mbox-names = "tx"; /* rx is optional */
>     shmem = <&cpu_scp_hpri>;
> };
>
> If you allow the client to provide the function ID (and I am not saying this is a good idea): where would this func ID come from? It would need to be a property of the client DT node, then. So one way would be to use the func ID as the Linux mailbox channel ID:
> mailbox: smc-mailbox {
>     #mbox-cells = <1>;
>     compatible = "arm,smc-mbox";
>     method = "smc";
> };
> scmi {
>     compatible = "arm,scmi";
>     mboxes = <&smc_mbox 0x820000fe>;
>     mbox-names = "tx"; /* rx is optional */
>     shmem = <&cpu_scp_hpri>;
> };
>
> But this doesn't look desirable.
>
> And as I mentioned this before: allowing some mailbox clients to provide the function IDs sound scary, as they could use anything they want, triggering random firmware actions (think PSCI_CPU_OFF).
>
That paranoia is unwarranted. We have to keep faith in kernel-space
code doing the right thing.
Either the illegitimate function request should be rejected by the
firmware or client driver be called buggy.... just as we would call a
block device driver buggy if it messed up the sector numbers in a
write request.

thnx.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
> 
> On Wed, Sep 11, 2019 at 10:03 AM Andre Przywara
> <andre.przywara@arm.com> wrote:
> >
> > On Tue, 10 Sep 2019 21:44:11 -0500
> > Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > Hi,
> >
> > > On Mon, Sep 9, 2019 at 10:42 AM Andre Przywara
> <andre.przywara@arm.com> wrote:
> > > >
> > > > On Wed, 28 Aug 2019 03:02:58 +0000 Peng Fan <peng.fan@nxp.com>
> > > > wrote:
> > > >
> > [ ... ]
> > > >
> > > > > +
> > > > > +  arm,func-ids:
> > > > > +    description: |
> > > > > +      An array of 32-bit values specifying the function IDs used by
> each
> > > > > +      mailbox channel. Those function IDs follow the ARM SMC
> calling
> > > > > +      convention standard [1].
> > > > > +
> > > > > +      There is one identifier per channel and the number of
> supported
> > > > > +      channels is determined by the length of this array.
> > > >
> > > > I think this makes it obvious that arm,num-chans is not needed.
> > > >
> > > > Also this somewhat contradicts the driver implementation, which allows
> the array to be shorter, marking this as UINT_MAX and later on using the first
> data item as a function identifier. This is somewhat surprising and not
> documented (unless I missed something).
> > > >
> > > > So I would suggest:
> > > > - We drop the transports property, and always put the client provided
> data in the registers, according to the SMCCC. Document this here.
> > > >   A client not needing those could always puts zeros (or garbage) in
> there, the respective firmware would just ignore the registers.
> > > > - We drop "arm,num-chans", as this is just redundant with the length of
> the func-ids array.
> > > > - We don't impose an arbitrary limit on the number of channels. From
> the firmware point of view this is just different function IDs, from Linux' point
> of view just the size of the memory used. Both don't need to be limited
> artificially IMHO.
> > > >
> > > Sounds like we are in sync.
> > >
> > > > - We mark arm,func-ids as required, as this needs to be fixed, allocated
> number.
> > > >
> > > I still think func-id can be done without. A client can always pass
> > > the value as it knows what it expects.
> >
> > I don't think it's the right abstraction. The mailbox *controller* uses a
> specific func-id, this has to match the one the firmware expects. So this is a
> property of the mailbox transport channel (the SMC call), and the *client*
> should *not* care about it. It just sees the logical channel ID (if we have one),
> which the controller translates into the func-ID.
> >
> arg0 is special only to the client/protocol, otherwise it is simply the first
> argument for the arm_smccc_smc *instruction* controller.
> arg[1,7] are already provided by the client, so it is only neater if
> arg0 is also taken from the client.
> 
> But as I said, I am still ok if func-id is passed from dt and arg0 from client is
> ignored because we have one channel per controller design and we don't have
> to worry about number of channels there can be dedicated to specific
> functions.

Ok, so I'll make it an optional property.

> 
> > So it should really look like this (assuming only single channel controllers):
> > mailbox: smc-mailbox {
> >     #mbox-cells = <0>;
> >     compatible = "arm,smc-mbox";
> >     method = "smc";
> >
> Do we want to do away with 'method' property and use different 'compatible'
> properties instead?
>  compatible = "arm,smc-mbox";     or    compatible = "arm,hvc-mbox";

I am ok, just need add data in driver to differentiate smc/hvc.
Andre, are you ok?

Thanks,
Peng.

> 
> >     arm,func-id = <0x820000fe>;
> > };
> > scmi {
> >     compatible = "arm,scmi";
> >     mboxes = <&smc_mbox>;
> >     mbox-names = "tx"; /* rx is optional */
> >     shmem = <&cpu_scp_hpri>;
> > };
> >
> > If you allow the client to provide the function ID (and I am not saying this is
> a good idea): where would this func ID come from? It would need to be a
> property of the client DT node, then. So one way would be to use the func ID
> as the Linux mailbox channel ID:
> > mailbox: smc-mailbox {
> >     #mbox-cells = <1>;
> >     compatible = "arm,smc-mbox";
> >     method = "smc";
> > };
> > scmi {
> >     compatible = "arm,scmi";
> >     mboxes = <&smc_mbox 0x820000fe>;
> >     mbox-names = "tx"; /* rx is optional */
> >     shmem = <&cpu_scp_hpri>;
> > };
> >
> > But this doesn't look desirable.
> >
> > And as I mentioned this before: allowing some mailbox clients to provide
> the function IDs sound scary, as they could use anything they want, triggering
> random firmware actions (think PSCI_CPU_OFF).
> >
> That paranoia is unwarranted. We have to keep faith in kernel-space code
> doing the right thing.
> Either the illegitimate function request should be rejected by the firmware or
> client driver be called buggy.... just as we would call a block device driver
> buggy if it messed up the sector numbers in a write request.
> 
> thnx.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel