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

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

[peng.fan]

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

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: mailbox: add binding doc for the ARM SMC mailbox
  mailbox: introduce ARM SMC based mailbox

 .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++
 drivers/mailbox/Kconfig                            |   7 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/arm-smc-mailbox.c                  | 190 +++++++++++++++++++++
 include/linux/mailbox/arm-smc-mailbox.h            |  10 ++
 5 files changed, 310 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c
 create mode 100644 include/linux/mailbox/arm-smc-mailbox.h

-- 
2.16.4


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

[PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[peng.fan]

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

The ARM SMC 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>
---

V2:
Introduce interrupts as a property.

V1:
arm,func-ids is still kept as an optional property, because there is no
defined SMC funciton id passed from SCMI. So in my test, I still use
arm,func-ids for ARM SIP service.

 .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt

diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
new file mode 100644
index 000000000000..401887118c09
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
@@ -0,0 +1,101 @@
+ARM SMC Mailbox Interface
+=========================
+
+This mailbox uses the ARM smc (secure monitor 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[1].
+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.
+
+Mailbox Device Node:
+====================
+
+This node is expected to be a child of the /firmware node.
+
+Required properties:
+--------------------
+- compatible:		Shall be "arm,smc-mbox"
+- #mbox-cells		Shall be 1 - the index of the channel needed.
+- arm,num-chans		The number of channels supported.
+- method:		A string, either:
+			"hvc": if the driver shall use an HVC call, or
+			"smc": if the driver shall use an SMC call.
+
+Optional properties:
+- arm,func-ids		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.
+- interrupts		SPI interrupts may be listed for notification,
+			each channel should use a dedicated interrupt
+			line.
+
+Example:
+--------
+
+	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>;
+		};
+	};
+
+	smc_mbox: mailbox {
+		#mbox-cells = <1>;
+		compatible = "arm,smc-mbox";
+		method = "smc";
+		arm,num-chans = <0x2>;
+		/* Optional */
+		arm,func-ids = <0xc20000fe>, <0xc20000ff>;
+	};
+
+	firmware {
+		scmi {
+			compatible = "arm,scmi";
+			mboxes = <&mailbox 0 &mailbox 1>;
+			mbox-names = "tx", "rx";
+			shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
+		};
+	};
+
+
+[1]
+http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0028a/index.html
-- 
2.16.4


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

[PATCH V2 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>
---

V2:
 Add interrupts notification support.

 drivers/mailbox/Kconfig                 |   7 ++
 drivers/mailbox/Makefile                |   2 +
 drivers/mailbox/arm-smc-mailbox.c       | 190 ++++++++++++++++++++++++++++++++
 include/linux/mailbox/arm-smc-mailbox.h |  10 ++
 4 files changed, 209 insertions(+)
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c
 create mode 100644 include/linux/mailbox/arm-smc-mailbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 595542bfae85..c3bd0f1ddcd8 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -15,6 +15,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..fef6e38d8b98
--- /dev/null
+++ b/drivers/mailbox/arm-smc-mailbox.c
@@ -0,0 +1,190 @@
+// 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/mailbox/arm-smc-mailbox.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define ARM_SMC_MBOX_USE_HVC	BIT(0)
+#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
+
+struct arm_smc_chan_data {
+	u32 function_id;
+	u32 flags;
+	int irq;
+};
+
+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;
+	struct arm_smccc_res res;
+	u32 function_id;
+
+	if (chan_data->function_id != UINT_MAX)
+		function_id = chan_data->function_id;
+	else
+		function_id = cmd->a0;
+
+	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
+		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+			      cmd->a5, cmd->a6, cmd->a7, &res);
+	else
+		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+			      cmd->a5, cmd->a6, cmd->a7, &res);
+
+	if (chan_data->irq)
+		return 0;
+
+	mbox_chan_received_data(link, (void *)res.a0);
+
+	return 0;
+}
+
+static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
+	.send_data	= arm_smc_send_data,
+};
+
+static irqreturn_t chan_irq_handler(int irq, void *data)
+{
+	struct mbox_chan *chan = data;
+
+	mbox_chan_received_data(chan, NULL);
+
+	return IRQ_HANDLED;
+}
+
+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 use_hvc = false;
+	int ret, irq_count, i;
+	u32 val;
+
+	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
+		if (val < 1 || val > INT_MAX) {
+			dev_err(dev, "invalid arm,num-chans value %u of %pOFn\n", val, pdev->dev.of_node);
+			return -EINVAL;
+		}
+	}
+
+	irq_count = platform_irq_count(pdev);
+	if (irq_count == -EPROBE_DEFER)
+		return irq_count;
+
+	if (irq_count && irq_count != val) {
+		dev_err(dev, "Interrupts not match num-chans\n");
+		return -EINVAL;
+	}
+
+	if (!of_property_read_string(dev->of_node, "method", &method)) {
+		if (!strcmp("hvc", method)) {
+			use_hvc = true;
+		} else if (!strcmp("smc", method)) {
+			use_hvc = false;
+		} else {
+			dev_warn(dev, "invalid \"method\" property: %s\n",
+				 method);
+
+			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;
+
+		if (use_hvc)
+			chan_data[i].flags |= ARM_SMC_MBOX_USE_HVC;
+		mbox->chans[i].con_priv = &chan_data[i];
+
+		if (irq_count) {
+			chan_data[i].irq = platform_get_irq(pdev, i);
+			if (chan_data[i].irq < 0)
+				return chan_data[i].irq;
+
+			ret = devm_request_irq(&pdev->dev, chan_data[i].irq,
+					       chan_irq_handler, 0, pdev->name,
+					       &mbox->chans[i]);
+			if (ret)
+				return ret;
+		}
+	}
+
+	mbox->txdone_poll = false;
+	mbox->txdone_irq = false;
+	mbox->ops = &arm_smc_mbox_chan_ops;
+	mbox->dev = dev;
+
+	ret = mbox_controller_register(mbox);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, mbox);
+	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");
diff --git a/include/linux/mailbox/arm-smc-mailbox.h b/include/linux/mailbox/arm-smc-mailbox.h
new file mode 100644
index 000000000000..ca366fe491c3
--- /dev/null
+++ b/include/linux/mailbox/arm-smc-mailbox.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_ARM_SMC_MAILBOX_H_
+#define _LINUX_ARM_SMC_MAILBOX_H_
+
+struct arm_smccc_mbox_cmd {
+	unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
+};
+
+#endif /* _LINUX_ARM_SMC_MAILBOX_H_ */
-- 
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 V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Florian Fainelli]

On 6/3/19 1:30 AM, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The ARM SMC 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>
> ---
> 
> V2:
> Introduce interrupts as a property.
> 
> V1:
> arm,func-ids is still kept as an optional property, because there is no
> defined SMC funciton id passed from SCMI. So in my test, I still use
> arm,func-ids for ARM SIP service.
> 
>  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> new file mode 100644
> index 000000000000..401887118c09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> @@ -0,0 +1,101 @@
> +ARM SMC Mailbox Interface
> +=========================
> +
> +This mailbox uses the ARM smc (secure monitor call) instruction to trigger
> +a mailbox-connected activity in firmware, executing on the very same core
> +as the caller.

The binding defines both hvc and smc as being valid methods for the mailbox.

> 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[1].
> +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.
> +
> +Mailbox Device Node:
> +====================
> +
> +This node is expected to be a child of the /firmware node.
> +
> +Required properties:
> +--------------------
> +- compatible:		Shall be "arm,smc-mbox"
> +- #mbox-cells		Shall be 1 - the index of the channel needed.
> +- arm,num-chans		The number of channels supported.
> +- method:		A string, either:
> +			"hvc": if the driver shall use an HVC call, or
> +			"smc": if the driver shall use an SMC call.
> +
> +Optional properties:
> +- arm,func-ids		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.
> +- interrupts		SPI interrupts may be listed for notification,
> +			each channel should use a dedicated interrupt
> +			line.

I would not go about defining a specific kind of interrupt, since SPI is
a GIC terminology, this firmware interface could be used in premise with
any parent interrupt controller, for which the concept of a SPI/PPI/SGI
may not be relevant.

> +
> +Example:
> +--------
> +
> +	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>;
> +		};
> +	};
> +
> +	smc_mbox: mailbox {
> +		#mbox-cells = <1>;
> +		compatible = "arm,smc-mbox";
> +		method = "smc";
> +		arm,num-chans = <0x2>;
> +		/* Optional */
> +		arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> +	};
> +
> +	firmware {
> +		scmi {
> +			compatible = "arm,scmi";
> +			mboxes = <&mailbox 0 &mailbox 1>;

It is visually nicer (and more consistent with yoyr arm,func-ids example
to use:
			mboxes = <&mailbox 0>, <&mailbox 1>;

> +			mbox-names = "tx", "rx";
> +			shmem = <&cpu_scp_lpri &cpu_scp_hpri>;

			shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;

Other than those, LGTM!
-- 
Florian

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

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

[Florian Fainelli]

On 6/3/19 1:30 AM, peng.fan@nxp.com 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>
> ---

[snip]

+#define ARM_SMC_MBOX_USB_IRQ	BIT(1)

That flag appears unused.

> +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 use_hvc = false;
> +	int ret, irq_count, i;
> +	u32 val;
> +
> +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> +		if (val < 1 || val > INT_MAX) {
> +			dev_err(dev, "invalid arm,num-chans value %u of %pOFn\n", val, pdev->dev.of_node);
> +			return -EINVAL;
> +		}
> +	}

Should not the upper bound check be done against UINT_MAX since val is
an unsigned int?

> +
> +	irq_count = platform_irq_count(pdev);
> +	if (irq_count == -EPROBE_DEFER)
> +		return irq_count;
> +
> +	if (irq_count && irq_count != val) {
> +		dev_err(dev, "Interrupts not match num-chans\n");

Interrupts property does not match \"arm,num-chans\" would be more correct.

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

Having at least one method specified does not seem to be checked later
on in the code, so if I omitted to specify that property, we would still
register the mailbox and default to use "smc" since the
ARM_SMC_MBOX_USE_HVC flag would not be set, would not we want to make
sure that we do have in fact a valid method specified given the binding
documents that property as mandatory?

[snip]

> +	mbox->txdone_poll = false;
> +	mbox->txdone_irq = false;
> +	mbox->ops = &arm_smc_mbox_chan_ops;
> +	mbox->dev = dev;
> +
> +	ret = mbox_controller_register(mbox);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, mbox);

I would move this above mbox_controller_register() that way there is no
room for race conditions in case another part of the driver expects to
have pdev->dev.drvdata set before the mbox controller is registered.
Since you use devm_* functions for everything, you may even remove that
call.

[snip]

> +#ifndef _LINUX_ARM_SMC_MAILBOX_H_
> +#define _LINUX_ARM_SMC_MAILBOX_H_
> +
> +struct arm_smccc_mbox_cmd {
> +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
> +};

Do you expect this to be used by other in-kernel users? If so, it might
be good to document how a0 can have a special meaning and be used as a
substitute for the function_id?
-- 
Florian

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

Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Sudeep Holla]

On Mon, Jun 03, 2019 at 09:22:16AM -0700, Florian Fainelli wrote:
> On 6/3/19 1:30 AM, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The ARM SMC 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>
> > ---
> >
> > V2:
> > Introduce interrupts as a property.
> >
> > V1:
> > arm,func-ids is still kept as an optional property, because there is no
> > defined SMC funciton id passed from SCMI. So in my test, I still use
> > arm,func-ids for ARM SIP service.
> >
> >  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > new file mode 100644
> > index 000000000000..401887118c09
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > @@ -0,0 +1,101 @@

[...]

> > +Optional properties:
> > +- arm,func-ids		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.
> > +- interrupts		SPI interrupts may be listed for notification,
> > +			each channel should use a dedicated interrupt
> > +			line.
>
> I would not go about defining a specific kind of interrupt, since SPI is
> a GIC terminology, this firmware interface could be used in premise with
> any parent interrupt controller, for which the concept of a SPI/PPI/SGI
> may not be relevant.
>

While I agree the binding document may not contain specifics, I still
don't see how to use SGI with this. Also note it's generally reserved
for OS future use(IPC) and using this for other than IPC may be bit
challenging IMO. It opens up lots of questions.

--
Regards,
Sudeep

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

Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Andre Przywara]

On Mon, 3 Jun 2019 17:56:51 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

Hi,

> On Mon, Jun 03, 2019 at 09:22:16AM -0700, Florian Fainelli wrote:
> > On 6/3/19 1:30 AM, peng.fan@nxp.com wrote:  
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The ARM SMC 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>
> > > ---
> > >
> > > V2:
> > > Introduce interrupts as a property.
> > >
> > > V1:
> > > arm,func-ids is still kept as an optional property, because there is no
> > > defined SMC funciton id passed from SCMI. So in my test, I still use
> > > arm,func-ids for ARM SIP service.
> > >
> > >  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
> > >  1 file changed, 101 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > new file mode 100644
> > > index 000000000000..401887118c09
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > @@ -0,0 +1,101 @@  
> 
> [...]
> 
> > > +Optional properties:
> > > +- arm,func-ids		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.
> > > +- interrupts		SPI interrupts may be listed for notification,
> > > +			each channel should use a dedicated interrupt
> > > +			line.  
> >
> > I would not go about defining a specific kind of interrupt, since SPI is
> > a GIC terminology, this firmware interface could be used in premise with
> > any parent interrupt controller, for which the concept of a SPI/PPI/SGI
> > may not be relevant.
> >  
> 
> While I agree the binding document may not contain specifics, I still
> don't see how to use SGI with this. Also note it's generally reserved
> for OS future use(IPC) and using this for other than IPC may be bit
> challenging IMO. It opens up lots of questions.

Well, a PPI might be possible to use, although it's somewhat dodgy to hijack the GIC's (re-)distributor from EL3 to write to GICD_ISPENDR<n>. Need to ask Marc about his feelings towards this. But it's definitely possible from a hypervisor to inject arbitrary interrupts into a guest.

But more importantly: is there any actual reason this needs to be a GIC interrupt? If I understand the code correctly, this could just be any interrupt, including one of an interrupt combiner or a GPIO chip. So why not just use the standard wording of: "exactly one interrupt specifier for each channel"?

By the way: Shouldn't new bindings use the YAML format instead?

Cheers,
Andre.

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

Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Florian Fainelli]

On 6/3/2019 10:18 AM, Andre Przywara wrote:
> On Mon, 3 Jun 2019 17:56:51 +0100
> Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> Hi,
> 
>> On Mon, Jun 03, 2019 at 09:22:16AM -0700, Florian Fainelli wrote:
>>> On 6/3/19 1:30 AM, peng.fan@nxp.com wrote:  
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> The ARM SMC 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>
>>>> ---
>>>>
>>>> V2:
>>>> Introduce interrupts as a property.
>>>>
>>>> V1:
>>>> arm,func-ids is still kept as an optional property, because there is no
>>>> defined SMC funciton id passed from SCMI. So in my test, I still use
>>>> arm,func-ids for ARM SIP service.
>>>>
>>>>  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
>>>>  1 file changed, 101 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
>>>> new file mode 100644
>>>> index 000000000000..401887118c09
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
>>>> @@ -0,0 +1,101 @@  
>>
>> [...]
>>
>>>> +Optional properties:
>>>> +- arm,func-ids		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.
>>>> +- interrupts		SPI interrupts may be listed for notification,
>>>> +			each channel should use a dedicated interrupt
>>>> +			line.  
>>>
>>> I would not go about defining a specific kind of interrupt, since SPI is
>>> a GIC terminology, this firmware interface could be used in premise with
>>> any parent interrupt controller, for which the concept of a SPI/PPI/SGI
>>> may not be relevant.
>>>  
>>
>> While I agree the binding document may not contain specifics, I still
>> don't see how to use SGI with this. Also note it's generally reserved
>> for OS future use(IPC) and using this for other than IPC may be bit
>> challenging IMO. It opens up lots of questions.
> 
> Well, a PPI might be possible to use, although it's somewhat dodgy to hijack the GIC's (re-)distributor from EL3 to write to GICD_ISPENDR<n>. Need to ask Marc about his feelings towards this. But it's definitely possible from a hypervisor to inject arbitrary interrupts into a guest.
> 
> But more importantly: is there any actual reason this needs to be a GIC interrupt? If I understand the code correctly, this could just be any interrupt, including one of an interrupt combiner or a GPIO chip. So why not just use the standard wording of: "exactly one interrupt specifier for each channel"?

That was my point, I am not stuck on using an SGI, or PPI, or anything
(even if that's what we have been using at the moment), any interrupt
would/should do here so the wording should be exactly as you indicated.
-- 
Florian

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

RE: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Peng Fan]

> Subject: Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC
> mailbox
> 
> On Mon, 3 Jun 2019 17:56:51 +0100
> Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> Hi,
> 
> > On Mon, Jun 03, 2019 at 09:22:16AM -0700, Florian Fainelli wrote:
> > > On 6/3/19 1:30 AM, peng.fan@nxp.com wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > The ARM SMC 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>
> > > > ---
> > > >
> > > > V2:
> > > > Introduce interrupts as a property.
> > > >
> > > > V1:
> > > > arm,func-ids is still kept as an optional property, because there
> > > > is no defined SMC funciton id passed from SCMI. So in my test, I
> > > > still use arm,func-ids for ARM SIP service.
> > > >
> > > >  .../devicetree/bindings/mailbox/arm-smc.txt        | 101
> +++++++++++++++++++++
> > > >  1 file changed, 101 insertions(+)  create mode 100644
> > > > Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > > b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > > new file mode 100644
> > > > index 000000000000..401887118c09
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > > @@ -0,0 +1,101 @@
> >
> > [...]
> >
> > > > +Optional properties:
> > > > +- arm,func-ids		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.
> > > > +- interrupts		SPI interrupts may be listed for notification,
> > > > +			each channel should use a dedicated interrupt
> > > > +			line.
> > >
> > > I would not go about defining a specific kind of interrupt, since
> > > SPI is a GIC terminology, this firmware interface could be used in
> > > premise with any parent interrupt controller, for which the concept
> > > of a SPI/PPI/SGI may not be relevant.
> > >
> >
> > While I agree the binding document may not contain specifics, I still
> > don't see how to use SGI with this. Also note it's generally reserved
> > for OS future use(IPC) and using this for other than IPC may be bit
> > challenging IMO. It opens up lots of questions.
> 
> Well, a PPI might be possible to use, although it's somewhat dodgy to hijack
> the GIC's (re-)distributor from EL3 to write to GICD_ISPENDR<n>. Need to ask
> Marc about his feelings towards this. But it's definitely possible from a
> hypervisor to inject arbitrary interrupts into a guest.
> 
> But more importantly: is there any actual reason this needs to be a GIC
> interrupt? 

No. I just test ATF with SPI when I posting out this. Should not restrict to be GIC.

If I understand the code correctly, this could just be any interrupt,
> including one of an interrupt combiner or a GPIO chip. So why not just use the
> standard wording of: "exactly one interrupt specifier for each channel"?

Agree.

> 
> By the way: Shouldn't new bindings use the YAML format instead?

I'll convert to YAML in next version.

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 V2 2/2] mailbox: introduce ARM SMC based mailbox

[Peng Fan]

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On 6/3/19 1:30 AM, peng.fan@nxp.com 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%7Caa396ba11ba244111fe408d6e8411fba%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636951763738548621&amp;sdata=UlNESNg7I7
> 4TM9xp%2F
> > VMce4CSbMuJ95lh68cQw%2FnQMOw%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> 
> [snip]
> 
> +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> 
> That flag appears unused.

I'll remove this in V3.

> 
> > +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 use_hvc = false;
> > +	int ret, irq_count, i;
> > +	u32 val;
> > +
> > +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> > +		if (val < 1 || val > INT_MAX) {
> > +			dev_err(dev, "invalid arm,num-chans value %u of %pOFn\n",
> val, pdev->dev.of_node);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> Should not the upper bound check be done against UINT_MAX since val is an
> unsigned int?

Fix in V3.

> 
> > +
> > +	irq_count = platform_irq_count(pdev);
> > +	if (irq_count == -EPROBE_DEFER)
> > +		return irq_count;
> > +
> > +	if (irq_count && irq_count != val) {
> > +		dev_err(dev, "Interrupts not match num-chans\n");
> 
> Interrupts property does not match \"arm,num-chans\" would be more
> correct.

Fix in V3.

> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> > +		if (!strcmp("hvc", method)) {
> > +			use_hvc = true;
> > +		} else if (!strcmp("smc", method)) {
> > +			use_hvc = false;
> > +		} else {
> > +			dev_warn(dev, "invalid \"method\" property: %s\n",
> > +				 method);
> > +
> > +			return -EINVAL;
> > +		}
> 
> Having at least one method specified does not seem to be checked later on in
> the code, so if I omitted to specify that property, we would still register the
> mailbox and default to use "smc" since the ARM_SMC_MBOX_USE_HVC flag
> would not be set, would not we want to make sure that we do have in fact a
> valid method specified given the binding documents that property as
> mandatory?

When arm_smc_send_data, it will check ARM_SMC_MBOX_USE_HVC,
you mean there are other places needs this flag check?

> 
> [snip]
> 
> > +	mbox->txdone_poll = false;
> > +	mbox->txdone_irq = false;
> > +	mbox->ops = &arm_smc_mbox_chan_ops;
> > +	mbox->dev = dev;
> > +
> > +	ret = mbox_controller_register(mbox);
> > +	if (ret)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, mbox);
> 
> I would move this above mbox_controller_register() that way there is no room
> for race conditions in case another part of the driver expects to have
> pdev->dev.drvdata set before the mbox controller is registered.

Right.

> Since you use devm_* functions for everything, you may even remove that
> call.

You mean remove " platform_set_drvdata(pdev, mbox);" ?

> 
> [snip]
> 
> > +#ifndef _LINUX_ARM_SMC_MAILBOX_H_
> > +#define _LINUX_ARM_SMC_MAILBOX_H_
> > +
> > +struct arm_smccc_mbox_cmd {
> > +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7; };
> 
> Do you expect this to be used by other in-kernel users? If so, it might be good
> to document how a0 can have a special meaning and be used as a substitute
> for the function_id?

This was to address comments here:
https://lore.kernel.org/patchwork/patch/812999/#1010433

Thanks,
Peng.

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

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

[Andre Przywara]

On Mon, 3 Jun 2019 09:32:42 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:

Hi,

> On 6/3/19 1:30 AM, peng.fan@nxp.com 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>
> > ---  
> 
> [snip]
> 
> +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> 
> That flag appears unused.
> 
> > +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 use_hvc = false;
> > +	int ret, irq_count, i;
> > +	u32 val;
> > +
> > +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> > +		if (val < 1 || val > INT_MAX) {
> > +			dev_err(dev, "invalid arm,num-chans value %u of %pOFn\n", val, pdev->dev.of_node);

Isn't the of_node parameter redundant, because dev_err() already takes care of that?

> > +			return -EINVAL;
> > +		}
> > +	}  
> 
> Should not the upper bound check be done against UINT_MAX since val is
> an unsigned int?

But wouldn't that be somewhat pointless, given that val is a u32? So I
guess we could just condense this down to:
...
		if (!val) {
...

> > +
> > +	irq_count = platform_irq_count(pdev);
> > +	if (irq_count == -EPROBE_DEFER)
> > +		return irq_count;
> > +
> > +	if (irq_count && irq_count != val) {
> > +		dev_err(dev, "Interrupts not match num-chans\n");  
> 
> Interrupts property does not match \"arm,num-chans\" would be more correct.

Given that interrupts are optional, do we have to rely on this? Do we
actually need one interrupt per channel?

> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> > +		if (!strcmp("hvc", method)) {
> > +			use_hvc = true;
> > +		} else if (!strcmp("smc", method)) {
> > +			use_hvc = false;
> > +		} else {
> > +			dev_warn(dev, "invalid \"method\" property: %s\n",
> > +				 method);
> > +
> > +			return -EINVAL;
> > +		}  
> 
> Having at least one method specified does not seem to be checked later
> on in the code, so if I omitted to specify that property, we would still
> register the mailbox and default to use "smc" since the
> ARM_SMC_MBOX_USE_HVC flag would not be set, would not we want to make
> sure that we do have in fact a valid method specified given the binding
> documents that property as mandatory?
> 
> [snip]
> 
> > +	mbox->txdone_poll = false;
> > +	mbox->txdone_irq = false;
> > +	mbox->ops = &arm_smc_mbox_chan_ops;
> > +	mbox->dev = dev;
> > +
> > +	ret = mbox_controller_register(mbox);
> > +	if (ret)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, mbox);  
> 
> I would move this above mbox_controller_register() that way there is no
> room for race conditions in case another part of the driver expects to
> have pdev->dev.drvdata set before the mbox controller is registered.
> Since you use devm_* functions for everything, you may even remove that
> call.
> 
> [snip]
> 
> > +#ifndef _LINUX_ARM_SMC_MAILBOX_H_
> > +#define _LINUX_ARM_SMC_MAILBOX_H_
> > +
> > +struct arm_smccc_mbox_cmd {
> > +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
> > +};  
> 
> Do you expect this to be used by other in-kernel users? If so, it might
> be good to document how a0 can have a special meaning and be used as a
> substitute for the function_id?

I don't think we should really expose this outside of the driver. From a mailbox point of view this is just the payload, transported according to the SMCCC. Also using "long" here sounds somewhat troublesome.

Also, looking at the SMCCC, I only see six parameters in addition to the function identifier. Shall we reflect this here?

Cheers,
Andre.


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

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

[Peng Fan]

Hi Andre,
> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, 3 Jun 2019 09:32:42 -0700
> Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> Hi,
> 
> > On 6/3/19 1:30 AM, peng.fan@nxp.com 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%2Flo
> > >
> re.kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%
> 7Cpen
> > >
> g.fan%40nxp.com%7C15c4180b8fe5405d3de808d6ea81d5f1%7C686ea1d3bc
> 2b4c6
> > >
> fa92cd99c5c301635%7C0%7C0%7C636954240720601454&amp;sdata=1Cp
> WSgTH7lF
> > > cBKxJnLeIDw%2FDAQJJO%2FVypV1LUU1BRQA%3D&amp;reserved=0
> > >
> > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> >
> > [snip]
> >
> > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> >
> > That flag appears unused.
> >
> > > +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 use_hvc = false;
> > > +	int ret, irq_count, i;
> > > +	u32 val;
> > > +
> > > +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> > > +		if (val < 1 || val > INT_MAX) {
> > > +			dev_err(dev, "invalid arm,num-chans value %u
> of %pOFn\n", val,
> > > +pdev->dev.of_node);
> 
> Isn't the of_node parameter redundant, because dev_err() already takes care
> of that?

I'll remove that.

> 
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> >
> > Should not the upper bound check be done against UINT_MAX since val is
> > an unsigned int?
> 
> But wouldn't that be somewhat pointless, given that val is a u32? So I guess
> we could just condense this down to:
> ...
> 		if (!val) {
> ...

make sense.

> 
> > > +
> > > +	irq_count = platform_irq_count(pdev);
> > > +	if (irq_count == -EPROBE_DEFER)
> > > +		return irq_count;
> > > +
> > > +	if (irq_count && irq_count != val) {
> > > +		dev_err(dev, "Interrupts not match num-chans\n");
> >
> > Interrupts property does not match \"arm,num-chans\" would be more
> correct.
> 
> Given that interrupts are optional, do we have to rely on this? 

If there is interrupt property, the interrupts should match channel counts.

Do we actually
> need one interrupt per channel?

I thought about this, provide one interrupt for all channels.
But there is no good way to let interrupt handlers know which
channel triggers the interrupt. So I use one interrupt per channel.

> 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> > > +		if (!strcmp("hvc", method)) {
> > > +			use_hvc = true;
> > > +		} else if (!strcmp("smc", method)) {
> > > +			use_hvc = false;
> > > +		} else {
> > > +			dev_warn(dev, "invalid \"method\" property: %s\n",
> > > +				 method);
> > > +
> > > +			return -EINVAL;
> > > +		}
> >
> > Having at least one method specified does not seem to be checked later
> > on in the code, so if I omitted to specify that property, we would
> > still register the mailbox and default to use "smc" since the
> > ARM_SMC_MBOX_USE_HVC flag would not be set, would not we want to
> make
> > sure that we do have in fact a valid method specified given the
> > binding documents that property as mandatory?
> >
> > [snip]
> >
> > > +	mbox->txdone_poll = false;
> > > +	mbox->txdone_irq = false;
> > > +	mbox->ops = &arm_smc_mbox_chan_ops;
> > > +	mbox->dev = dev;
> > > +
> > > +	ret = mbox_controller_register(mbox);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	platform_set_drvdata(pdev, mbox);
> >
> > I would move this above mbox_controller_register() that way there is
> > no room for race conditions in case another part of the driver expects
> > to have pdev->dev.drvdata set before the mbox controller is registered.
> > Since you use devm_* functions for everything, you may even remove
> > that call.
> >
> > [snip]
> >
> > > +#ifndef _LINUX_ARM_SMC_MAILBOX_H_
> > > +#define _LINUX_ARM_SMC_MAILBOX_H_
> > > +
> > > +struct arm_smccc_mbox_cmd {
> > > +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7; };
> >
> > Do you expect this to be used by other in-kernel users? If so, it
> > might be good to document how a0 can have a special meaning and be
> > used as a substitute for the function_id?
> 
> I don't think we should really expose this outside of the driver. From a mailbox
> point of view this is just the payload, transported according to the SMCCC.
> Also using "long" here sounds somewhat troublesome.
> 
> Also, looking at the SMCCC, I only see six parameters in addition to the
> function identifier. Shall we reflect this here?

I could move it to driver code. Jassi, do you have any comments?

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 V2 2/2] mailbox: introduce ARM SMC based mailbox

[Andre Przywara]

On Mon, 10 Jun 2019 01:32:49 +0000
Peng Fan <peng.fan@nxp.com> wrote:

Hi Peng,

[ ... ]

> > > > +
> > > > +	irq_count = platform_irq_count(pdev);
> > > > +	if (irq_count == -EPROBE_DEFER)
> > > > +		return irq_count;
> > > > +
> > > > +	if (irq_count && irq_count != val) {
> > > > +		dev_err(dev, "Interrupts not match num-chans\n");  
> > >
> > > Interrupts property does not match \"arm,num-chans\" would be more  
> > correct.
> > 
> > Given that interrupts are optional, do we have to rely on this?   
> 
> If there is interrupt property, the interrupts should match channel counts.
> 
> Do we actually
> > need one interrupt per channel?  
> 
> I thought about this, provide one interrupt for all channels.
> But there is no good way to let interrupt handlers know which
> channel triggers the interrupt. So I use one interrupt per channel.

Yeah, I was wondering about this as well. Seems like we need this indeed.
Just sounds wasteful, but I guess we don't expect many channels anyway,
normally.

Cheers,
Andre.

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

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

[Peng Fan]

Hi Andre,

> Subject: RE: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> Hi Andre,
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Mon, 3 Jun 2019 09:32:42 -0700
> > Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > Hi,
> >
> > > On 6/3/19 1:30 AM, peng.fan@nxp.com 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%2F
> > > > lo
> > > >
> >
> re.kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%
> > 7Cpen
> > > >
> >
> g.fan%40nxp.com%7C15c4180b8fe5405d3de808d6ea81d5f1%7C686ea1d3bc
> > 2b4c6
> > > >
> > fa92cd99c5c301635%7C0%7C0%7C636954240720601454&amp;sdata=1Cp
> > WSgTH7lF
> > > > cBKxJnLeIDw%2FDAQJJO%2FVypV1LUU1BRQA%3D&amp;reserved=0
> > > >
> > > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > >
> > > [snip]
> > >
> > > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > >
> > > That flag appears unused.
> > >
> > > > +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 use_hvc = false;
> > > > +	int ret, irq_count, i;
> > > > +	u32 val;
> > > > +
> > > > +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> > > > +		if (val < 1 || val > INT_MAX) {
> > > > +			dev_err(dev, "invalid arm,num-chans value %u
> > of %pOFn\n", val,
> > > > +pdev->dev.of_node);
> >
> > Isn't the of_node parameter redundant, because dev_err() already takes
> > care of that?
> 
> I'll remove that.
> 
> >
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > >
> > > Should not the upper bound check be done against UINT_MAX since val
> > > is an unsigned int?
> >
> > But wouldn't that be somewhat pointless, given that val is a u32? So I
> > guess we could just condense this down to:
> > ...
> > 		if (!val) {
> > ...
> 
> make sense.
> 
> >
> > > > +
> > > > +	irq_count = platform_irq_count(pdev);
> > > > +	if (irq_count == -EPROBE_DEFER)
> > > > +		return irq_count;
> > > > +
> > > > +	if (irq_count && irq_count != val) {
> > > > +		dev_err(dev, "Interrupts not match num-chans\n");
> > >
> > > Interrupts property does not match \"arm,num-chans\" would be more
> > correct.
> >
> > Given that interrupts are optional, do we have to rely on this?
> 
> If there is interrupt property, the interrupts should match channel counts.
> 
> Do we actually
> > need one interrupt per channel?
> 
> I thought about this, provide one interrupt for all channels.
> But there is no good way to let interrupt handlers know which channel
> triggers the interrupt. So I use one interrupt per channel.
> 
> >
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> > > > +		if (!strcmp("hvc", method)) {
> > > > +			use_hvc = true;
> > > > +		} else if (!strcmp("smc", method)) {
> > > > +			use_hvc = false;
> > > > +		} else {
> > > > +			dev_warn(dev, "invalid \"method\" property: %s\n",
> > > > +				 method);
> > > > +
> > > > +			return -EINVAL;
> > > > +		}
> > >
> > > Having at least one method specified does not seem to be checked
> > > later on in the code, so if I omitted to specify that property, we
> > > would still register the mailbox and default to use "smc" since the
> > > ARM_SMC_MBOX_USE_HVC flag would not be set, would not we want to
> > make
> > > sure that we do have in fact a valid method specified given the
> > > binding documents that property as mandatory?
> > >
> > > [snip]
> > >
> > > > +	mbox->txdone_poll = false;
> > > > +	mbox->txdone_irq = false;
> > > > +	mbox->ops = &arm_smc_mbox_chan_ops;
> > > > +	mbox->dev = dev;
> > > > +
> > > > +	ret = mbox_controller_register(mbox);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	platform_set_drvdata(pdev, mbox);
> > >
> > > I would move this above mbox_controller_register() that way there is
> > > no room for race conditions in case another part of the driver
> > > expects to have pdev->dev.drvdata set before the mbox controller is
> registered.
> > > Since you use devm_* functions for everything, you may even remove
> > > that call.
> > >
> > > [snip]
> > >
> > > > +#ifndef _LINUX_ARM_SMC_MAILBOX_H_ #define
> > > > +_LINUX_ARM_SMC_MAILBOX_H_
> > > > +
> > > > +struct arm_smccc_mbox_cmd {
> > > > +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7; };
> > >
> > > Do you expect this to be used by other in-kernel users? If so, it
> > > might be good to document how a0 can have a special meaning and be
> > > used as a substitute for the function_id?
> >
> > I don't think we should really expose this outside of the driver. From
> > a mailbox point of view this is just the payload, transported according to the
> SMCCC.
> > Also using "long" here sounds somewhat troublesome.

Long on ARM64 is 64bit, and 32bit on ARM32, so I use long.
Do you forsee any issues?

> >
> > Also, looking at the SMCCC, I only see six parameters in addition to
> > the function identifier. Shall we reflect this here?

a0 is used as function id, not no arm,func-ids provided in dts. a1-a7 are
also passed to smc.
If arm,func-ids is provided, a0 will be omitted just for consistency as above.

You mean write comments in the code for it?

Thanks,
Peng.

> 
> I could move it to driver code. Jassi, do you have any comments?
> 
> 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 V2 2/2] mailbox: introduce ARM SMC based mailbox

[Andre Przywara]

On Wed, 12 Jun 2019 12:59:04 +0000
Peng Fan <peng.fan@nxp.com> wrote:

Hi,

> > Subject: RE: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > 
> > Hi Andre,  
> > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > >
> > > On Mon, 3 Jun 2019 09:32:42 -0700
> > > Florian Fainelli <f.fainelli@gmail.com> wrote:
> > >
> > > Hi,
> > >  
> > > > On 6/3/19 1:30 AM, peng.fan@nxp.com 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%2F
> > > > > lo
> > > > >  
> > >  
> > re.kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%  
> > > 7Cpen  
> > > > >  
> > >  
> > g.fan%40nxp.com%7C15c4180b8fe5405d3de808d6ea81d5f1%7C686ea1d3bc  
> > > 2b4c6  
> > > > >  
> > > fa92cd99c5c301635%7C0%7C0%7C636954240720601454&amp;sdata=1Cp
> > > WSgTH7lF  
> > > > > cBKxJnLeIDw%2FDAQJJO%2FVypV1LUU1BRQA%3D&amp;reserved=0
> > > > >
> > > > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---  
> > > >
> > > > [snip]
> > > >
> > > > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > > >
> > > > That flag appears unused.
> > > >  
> > > > > +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 use_hvc = false;
> > > > > +	int ret, irq_count, i;
> > > > > +	u32 val;
> > > > > +
> > > > > +	if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> > > > > +		if (val < 1 || val > INT_MAX) {
> > > > > +			dev_err(dev, "invalid arm,num-chans value %u  
> > > of %pOFn\n", val,  
> > > > > +pdev->dev.of_node);  
> > >
> > > Isn't the of_node parameter redundant, because dev_err() already takes
> > > care of that?  
> > 
> > I'll remove that.
> >   
> > >  
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +	}  
> > > >
> > > > Should not the upper bound check be done against UINT_MAX since val
> > > > is an unsigned int?  
> > >
> > > But wouldn't that be somewhat pointless, given that val is a u32? So I
> > > guess we could just condense this down to:
> > > ...
> > > 		if (!val) {
> > > ...  
> > 
> > make sense.
> >   
> > >  
> > > > > +
> > > > > +	irq_count = platform_irq_count(pdev);
> > > > > +	if (irq_count == -EPROBE_DEFER)
> > > > > +		return irq_count;
> > > > > +
> > > > > +	if (irq_count && irq_count != val) {
> > > > > +		dev_err(dev, "Interrupts not match num-chans\n");  
> > > >
> > > > Interrupts property does not match \"arm,num-chans\" would be more  
> > > correct.
> > >
> > > Given that interrupts are optional, do we have to rely on this?  
> > 
> > If there is interrupt property, the interrupts should match channel counts.
> > 
> > Do we actually  
> > > need one interrupt per channel?  
> > 
> > I thought about this, provide one interrupt for all channels.
> > But there is no good way to let interrupt handlers know which channel
> > triggers the interrupt. So I use one interrupt per channel.
> >   
> > >  
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	if (!of_property_read_string(dev->of_node, "method", &method)) {
> > > > > +		if (!strcmp("hvc", method)) {
> > > > > +			use_hvc = true;
> > > > > +		} else if (!strcmp("smc", method)) {
> > > > > +			use_hvc = false;
> > > > > +		} else {
> > > > > +			dev_warn(dev, "invalid \"method\" property: %s\n",
> > > > > +				 method);
> > > > > +
> > > > > +			return -EINVAL;
> > > > > +		}  
> > > >
> > > > Having at least one method specified does not seem to be checked
> > > > later on in the code, so if I omitted to specify that property, we
> > > > would still register the mailbox and default to use "smc" since the
> > > > ARM_SMC_MBOX_USE_HVC flag would not be set, would not we want to  
> > > make  
> > > > sure that we do have in fact a valid method specified given the
> > > > binding documents that property as mandatory?
> > > >
> > > > [snip]
> > > >  
> > > > > +	mbox->txdone_poll = false;
> > > > > +	mbox->txdone_irq = false;
> > > > > +	mbox->ops = &arm_smc_mbox_chan_ops;
> > > > > +	mbox->dev = dev;
> > > > > +
> > > > > +	ret = mbox_controller_register(mbox);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	platform_set_drvdata(pdev, mbox);  
> > > >
> > > > I would move this above mbox_controller_register() that way there is
> > > > no room for race conditions in case another part of the driver
> > > > expects to have pdev->dev.drvdata set before the mbox controller is  
> > registered.  
> > > > Since you use devm_* functions for everything, you may even remove
> > > > that call.
> > > >
> > > > [snip]
> > > >  
> > > > > +#ifndef _LINUX_ARM_SMC_MAILBOX_H_ #define
> > > > > +_LINUX_ARM_SMC_MAILBOX_H_
> > > > > +
> > > > > +struct arm_smccc_mbox_cmd {
> > > > > +	unsigned long a0, a1, a2, a3, a4, a5, a6, a7; };  
> > > >
> > > > Do you expect this to be used by other in-kernel users? If so, it
> > > > might be good to document how a0 can have a special meaning and be
> > > > used as a substitute for the function_id?  
> > >
> > > I don't think we should really expose this outside of the driver. From
> > > a mailbox point of view this is just the payload, transported according to the  
> > SMCCC.  
> > > Also using "long" here sounds somewhat troublesome.  
> 
> Long on ARM64 is 64bit, and 32bit on ARM32, so I use long.
> Do you forsee any issues?

Yes, because having different sizes depending on the underlying instruction set asks for trouble when talking about protocols. If I compile a kernel with this driver once for arm(32) and once for arm64 and run it on the same machine with the same firmware, does that behave differently? Not saying it's impossible to handle, but we should make sure there is no ambiguity.

> > > Also, looking at the SMCCC, I only see six parameters in addition to
> > > the function identifier. Shall we reflect this here?  
> 
> a0 is used as function id, not no arm,func-ids provided in dts. a1-a7 are
> also passed to smc.

Yes, so those are *7* parameters on top of the function IDs, whereas the SMCCC only speaks of 6:
"When the SMC64/HVC64 convention is used, the SMC or HVC instruction takes up to six 64-bit arguments in
registers ..."
"When the SMC32/HVC32 convention is used, an SMC or HVC instruction takes a Function Identifier and up to six
32-bit register values as arguments, ..."

> If arm,func-ids is provided, a0 will be omitted just for consistency as above.
> 
> You mean write comments in the code for it?

I think we should prevent people expecting anything useful to happen with the seventh argument.

Cheers,
Andre

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

Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Sudeep Holla]

On Mon, Jun 03, 2019 at 04:30:04PM +0800, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The ARM SMC 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>
> ---
> 
> V2:
> Introduce interrupts as a property.
> 
> V1:
> arm,func-ids is still kept as an optional property, because there is no
> defined SMC funciton id passed from SCMI. So in my test, I still use
> arm,func-ids for ARM SIP service.
> 
>  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> new file mode 100644
> index 000000000000..401887118c09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> @@ -0,0 +1,101 @@
> +ARM SMC Mailbox Interface
> +=========================
> +
> +This mailbox uses the ARM smc (secure monitor 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[1].
> +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.
> +
> +Mailbox Device Node:
> +====================
> +
> +This node is expected to be a child of the /firmware node.
> +
> +Required properties:
> +--------------------
> +- compatible:		Shall be "arm,smc-mbox"
> +- #mbox-cells		Shall be 1 - the index of the channel needed.
> +- arm,num-chans		The number of channels supported.
> +- method:		A string, either:
> +			"hvc": if the driver shall use an HVC call, or
> +			"smc": if the driver shall use an SMC call.
> +
> +Optional properties:
> +- arm,func-ids		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.
> +- interrupts		SPI interrupts may be listed for notification,
> +			each channel should use a dedicated interrupt
> +			line.
> +

I think SMC mailbox as mostly unidirectional/Tx only channel. And the
interrupts here as stated are for notifications, so I prefer to keep
them separate channel. I assume SMC call return indicates completion.
Or do you plan to use these interrupts as the indication for completion
of the command? I see in patch 2/2 the absence of IRQ is anyway dealt
the way I mention above.

Does it make sense or am I missing something here ?

--
Regards,
Sudeep

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

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

[Sudeep Holla]

On Mon, Jun 03, 2019 at 04:30:05PM +0800, peng.fan@nxp.com 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>
> ---
>
> V2:
>  Add interrupts notification support.
>
>  drivers/mailbox/Kconfig                 |   7 ++
>  drivers/mailbox/Makefile                |   2 +
>  drivers/mailbox/arm-smc-mailbox.c       | 190 ++++++++++++++++++++++++++++++++
>  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
>  4 files changed, 209 insertions(+)
>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>  create mode 100644 include/linux/mailbox/arm-smc-mailbox.h
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 595542bfae85..c3bd0f1ddcd8 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,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..fef6e38d8b98
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,190 @@
> +// 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/mailbox/arm-smc-mailbox.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> +
> +struct arm_smc_chan_data {
> +	u32 function_id;
> +	u32 flags;
> +	int irq;
> +};
> +
> +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;
> +	struct arm_smccc_res res;
> +	u32 function_id;
> +
> +	if (chan_data->function_id != UINT_MAX)
> +		function_id = chan_data->function_id;
> +	else
> +		function_id = cmd->a0;
> +
> +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +			      cmd->a5, cmd->a6, cmd->a7, &res);
> +	else
> +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +			      cmd->a5, cmd->a6, cmd->a7, &res);
> +

So how will the SMC/HVC handler in EL3/2 find which mailbox is being referred
with this command ? I prefer 2nd argument to be the mailbox number.

--
Regards,
Sudeep

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

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

[Peng Fan]

Hi Sudeep,

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, Jun 03, 2019 at 04:30:05PM +0800, peng.fan@nxp.com 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%7C6b37f78032e446be750e08d6f560e707%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636966193913988679&amp;sdata=UNM4MTPs
> brqoMqWStEy
> > YzzwMEWTmX7hHO3TeNEz%2BOAw%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  Add interrupts notification support.
> >
> >  drivers/mailbox/Kconfig                 |   7 ++
> >  drivers/mailbox/Makefile                |   2 +
> >  drivers/mailbox/arm-smc-mailbox.c       | 190
> ++++++++++++++++++++++++++++++++
> >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> >  4 files changed, 209 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > 100644 include/linux/mailbox/arm-smc-mailbox.h
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > 595542bfae85..c3bd0f1ddcd8 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,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..fef6e38d8b98
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,190 @@
> > +// 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/mailbox/arm-smc-mailbox.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > +
> > +struct arm_smc_chan_data {
> > +	u32 function_id;
> > +	u32 flags;
> > +	int irq;
> > +};
> > +
> > +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;
> > +	struct arm_smccc_res res;
> > +	u32 function_id;
> > +
> > +	if (chan_data->function_id != UINT_MAX)
> > +		function_id = chan_data->function_id;
> > +	else
> > +		function_id = cmd->a0;
> > +
> > +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3,
> cmd->a4,
> > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > +	else
> > +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3,
> cmd->a4,
> > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > +
> 
> So how will the SMC/HVC handler in EL3/2 find which mailbox is being
> referred with this command ? I prefer 2nd argument to be the mailbox
> number.
You mean channel number as following?

@@ -37,10 +38,10 @@ static int arm_smc_send_data(struct mbox_chan *link, void *data)
                function_id = cmd->a0;

        if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
-               arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+               arm_smccc_hvc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
                              cmd->a5, cmd->a6, cmd->a7, &res);
        else
-               arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+               arm_smccc_smc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
                              cmd->a5, cmd->a6, cmd->a7, &res);

Or should that be passed from firmware driver?

If not from firmware driver, just as above, I do not have a good idea which should be passed to smc,
from cmd->a1 to a5 or from cmd->a2 to a6.

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 V2 2/2] mailbox: introduce ARM SMC based mailbox

[Sudeep Holla]

On Thu, Jun 20, 2019 at 10:21:09AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Mon, Jun 03, 2019 at 04:30:05PM +0800, peng.fan@nxp.com 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%7C6b37f78032e446be750e08d6f560e707%7C686ea1d3bc2b4
> > c6fa92cd
> > >
> > 99c5c301635%7C0%7C0%7C636966193913988679&amp;sdata=UNM4MTPs
> > brqoMqWStEy
> > > YzzwMEWTmX7hHO3TeNEz%2BOAw%3D&amp;reserved=0
> > >
> > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >
> > > V2:
> > >  Add interrupts notification support.
> > >
> > >  drivers/mailbox/Kconfig                 |   7 ++
> > >  drivers/mailbox/Makefile                |   2 +
> > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > ++++++++++++++++++++++++++++++++
> > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > >  4 files changed, 209 insertions(+)
> > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > 595542bfae85..c3bd0f1ddcd8 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -15,6 +15,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..fef6e38d8b98
> > > --- /dev/null
> > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > @@ -0,0 +1,190 @@
> > > +// 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/mailbox/arm-smc-mailbox.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> > > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > > +
> > > +struct arm_smc_chan_data {
> > > +	u32 function_id;
> > > +	u32 flags;
> > > +	int irq;
> > > +};
> > > +
> > > +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;
> > > +	struct arm_smccc_res res;
> > > +	u32 function_id;
> > > +
> > > +	if (chan_data->function_id != UINT_MAX)
> > > +		function_id = chan_data->function_id;
> > > +	else
> > > +		function_id = cmd->a0;
> > > +
> > > +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > > +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > cmd->a4,
> > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > +	else
> > > +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > cmd->a4,
> > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > +
> >
> > So how will the SMC/HVC handler in EL3/2 find which mailbox is being
> > referred with this command ? I prefer 2nd argument to be the mailbox
> > number.
> You mean channel number as following?
>
> @@ -37,10 +38,10 @@ static int arm_smc_send_data(struct mbox_chan *link, void *data)
>                 function_id = cmd->a0;
>
>         if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> -               arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +               arm_smccc_hvc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
>                               cmd->a5, cmd->a6, cmd->a7, &res);
>         else
> -               arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +               arm_smccc_smc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
>                               cmd->a5, cmd->a6, cmd->a7, &res);
>

Yes something like above. There's a brief description of the same in
latest SCMI specification though it's not related to SCMI, it more 
general note for SMC based mailbox.

"In case the doorbell is SMC/HVC based, it should follow the SMC Calling
Convention [SMCCC] and needs to provide the identifier of the Shared Memory
area that contains the payload. On return from the call, the Shared Memory
area which contained the payload is now updated with the SCMI return response.
The identifier of the Shared Memory area should be 32-bits and each identifier
should denote a distinct Shared Memory area."

> Or should that be passed from firmware driver?
>

No, we can't assume the id's in DT are 1-1 translation to mailbox ID used
though it may be the same most of the time.

> If not from firmware driver, just as above, I do not have a good idea which
> should be passed to smc, from cmd->a1 to a5 or from cmd->a2 to a6.
>

Also I found copying those registers may not be always needed and can
be sub-optimal. May be a way to indicate that this in DT whether
register based transfers are used or using memory. Just a thought.

--
Regards,
Sudeep

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

Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Andre Przywara]

On Thu, 20 Jun 2019 10:22:41 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> On Mon, Jun 03, 2019 at 04:30:04PM +0800, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > The ARM SMC 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>
> > ---
> > 
> > V2:
> > Introduce interrupts as a property.
> > 
> > V1:
> > arm,func-ids is still kept as an optional property, because there is no
> > defined SMC funciton id passed from SCMI. So in my test, I still use
> > arm,func-ids for ARM SIP service.
> > 
> >  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > new file mode 100644
> > index 000000000000..401887118c09
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > @@ -0,0 +1,101 @@
> > +ARM SMC Mailbox Interface
> > +=========================
> > +
> > +This mailbox uses the ARM smc (secure monitor 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[1].
> > +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.
> > +
> > +Mailbox Device Node:
> > +====================
> > +
> > +This node is expected to be a child of the /firmware node.
> > +
> > +Required properties:
> > +--------------------
> > +- compatible:		Shall be "arm,smc-mbox"
> > +- #mbox-cells		Shall be 1 - the index of the channel needed.
> > +- arm,num-chans		The number of channels supported.
> > +- method:		A string, either:
> > +			"hvc": if the driver shall use an HVC call, or
> > +			"smc": if the driver shall use an SMC call.
> > +
> > +Optional properties:
> > +- arm,func-ids		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.
> > +- interrupts		SPI interrupts may be listed for notification,
> > +			each channel should use a dedicated interrupt
> > +			line.
> > +  
> 
> I think SMC mailbox as mostly unidirectional/Tx only channel. And the
> interrupts here as stated are for notifications, so I prefer to keep
> them separate channel. I assume SMC call return indicates completion.
> Or do you plan to use these interrupts as the indication for completion
> of the command? I see in patch 2/2 the absence of IRQ is anyway dealt
> the way I mention above.
> 
> Does it make sense or am I missing something here ?

I think you are right. From a mailbox point of view "completion" means
that the trigger has reached the other side. A returning smc call is a
perfect indication of this fact. Whether the action triggered by this
mailbox command has completed is a totally separate question and out of
the scope of the mailbox. This should be handled by a higher level
protocol (SCPI in this case). Which could mean that this employs a
separate return mailbox channel, which is RX only and implemented by
interrupts. Which could or could not be part of this driver.

Cheers,
Andre

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

Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Jassi Brar]

On Thu, Jun 20, 2019 at 11:13 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 20 Jun 2019 10:22:41 +0100
> Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> > On Mon, Jun 03, 2019 at 04:30:04PM +0800, peng.fan@nxp.com wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The ARM SMC 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>
> > > ---
> > >
> > > V2:
> > > Introduce interrupts as a property.
> > >
> > > V1:
> > > arm,func-ids is still kept as an optional property, because there is no
> > > defined SMC funciton id passed from SCMI. So in my test, I still use
> > > arm,func-ids for ARM SIP service.
> > >
> > >  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
> > >  1 file changed, 101 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > new file mode 100644
> > > index 000000000000..401887118c09
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > @@ -0,0 +1,101 @@
> > > +ARM SMC Mailbox Interface
> > > +=========================
> > > +
> > > +This mailbox uses the ARM smc (secure monitor 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[1].
> > > +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.
> > > +
> > > +Mailbox Device Node:
> > > +====================
> > > +
> > > +This node is expected to be a child of the /firmware node.
> > > +
> > > +Required properties:
> > > +--------------------
> > > +- compatible:              Shall be "arm,smc-mbox"
> > > +- #mbox-cells              Shall be 1 - the index of the channel needed.
> > > +- arm,num-chans            The number of channels supported.
> > > +- method:          A string, either:
> > > +                   "hvc": if the driver shall use an HVC call, or
> > > +                   "smc": if the driver shall use an SMC call.
> > > +
> > > +Optional properties:
> > > +- arm,func-ids             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.
> > > +- interrupts               SPI interrupts may be listed for notification,
> > > +                   each channel should use a dedicated interrupt
> > > +                   line.
> > > +
> >
> > I think SMC mailbox as mostly unidirectional/Tx only channel. And the
> > interrupts here as stated are for notifications, so I prefer to keep
> > them separate channel. I assume SMC call return indicates completion.
> > Or do you plan to use these interrupts as the indication for completion
> > of the command? I see in patch 2/2 the absence of IRQ is anyway dealt
> > the way I mention above.
> >
> > Does it make sense or am I missing something here ?
>
> I think you are right. From a mailbox point of view "completion" means
> that the trigger has reached the other side. A returning smc call is a
> perfect indication of this fact.
>
Yes. mailbox only cares about message delivery.

> Whether the action triggered by this
> mailbox command has completed is a totally separate question and out of
> the scope of the mailbox.
>
Yes, whether the message is accepted/rejected at protocol level is a
matter of upper layer (protocol).

> This should be handled by a higher level
> protocol (SCPI in this case). Which could mean that this employs a
> separate return mailbox channel, which is RX only and implemented by
> interrupts. Which could or could not be part of this driver.
>
Any message received over the same class of channel should be handled
in this driver.

Cheers

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

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

[Jassi Brar]

On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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>
> ---
>
> V2:
>  Add interrupts notification support.
>
>  drivers/mailbox/Kconfig                 |   7 ++
>  drivers/mailbox/Makefile                |   2 +
>  drivers/mailbox/arm-smc-mailbox.c       | 190 ++++++++++++++++++++++++++++++++
>  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
>  4 files changed, 209 insertions(+)
>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>  create mode 100644 include/linux/mailbox/arm-smc-mailbox.h
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 595542bfae85..c3bd0f1ddcd8 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,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..fef6e38d8b98
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,190 @@
> +// 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/mailbox/arm-smc-mailbox.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> +
IRQ bit is unused (and unnecessary IMO)

> +struct arm_smc_chan_data {
> +       u32 function_id;
> +       u32 flags;
> +       int irq;
> +};
> +
> +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;
> +       struct arm_smccc_res res;
> +       u32 function_id;
> +
> +       if (chan_data->function_id != UINT_MAX)
> +               function_id = chan_data->function_id;
> +       else
> +               function_id = cmd->a0;
> +
Not sure about chan_data->function_id.  Why restrict from DT?
'a0' is the function_id register, let the user pass func-id via the
'a0' like other values via 'a[1-7]'


> +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +                             cmd->a5, cmd->a6, cmd->a7, &res);
> +       else
> +               arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +                             cmd->a5, cmd->a6, cmd->a7, &res);
> +
> +       if (chan_data->irq)
> +               return 0;
> +
This irq thing seems like oob signalling, that is, a protocol thing.
And then it provides lesser info via chan_irq_handler (returns NULL)
than res.a0 - which can always be ignored if not needed.
So the irq should be implemented in the upper layer if the protocol needs it.

> +       mbox_chan_received_data(link, (void *)res.a0);
> +
This is fine.

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

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

[Peng Fan]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: 2019年6月21日 0:50
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; Florian
> Fainelli <f.fainelli@gmail.com>; , Sascha Hauer <kernel@pengutronix.de>;
> dl-linux-imx <linux-imx@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> festevam@gmail.com; Devicetree List <devicetree@vger.kernel.org>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>;
> linux-arm-kernel@lists.infradead.org; Andre Przywara
> <andre.przywara@arm.com>; van.freenix@gmail.com
> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> ZkeRMtL8Bx
> > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  Add interrupts notification support.
> >
> >  drivers/mailbox/Kconfig                 |   7 ++
> >  drivers/mailbox/Makefile                |   2 +
> >  drivers/mailbox/arm-smc-mailbox.c       | 190
> ++++++++++++++++++++++++++++++++
> >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> >  4 files changed, 209 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > 100644 include/linux/mailbox/arm-smc-mailbox.h
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > 595542bfae85..c3bd0f1ddcd8 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,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..fef6e38d8b98
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,190 @@
> > +// 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/mailbox/arm-smc-mailbox.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > +
> IRQ bit is unused (and unnecessary IMO)

This will be removed in next version.

> 
> > +struct arm_smc_chan_data {
> > +       u32 function_id;
> > +       u32 flags;
> > +       int irq;
> > +};
> > +
> > +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;
> > +       struct arm_smccc_res res;
> > +       u32 function_id;
> > +
> > +       if (chan_data->function_id != UINT_MAX)
> > +               function_id = chan_data->function_id;
> > +       else
> > +               function_id = cmd->a0;
> > +
> Not sure about chan_data->function_id.  Why restrict from DT?
> 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
> values via 'a[1-7]'
> 
> 
> > +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +       else
> > +               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +
> > +       if (chan_data->irq)
> > +               return 0;
> > +
> This irq thing seems like oob signalling, that is, a protocol thing.
> And then it provides lesser info via chan_irq_handler (returns NULL) than
> res.a0 - which can always be ignored if not needed.
> So the irq should be implemented in the upper layer if the protocol needs it.

The interrupts was added here because in v1, Florian suggest
"
I would just put a
provision in the binding to support an optional interrupt such that
asynchronism gets reasonably easy to plug in when it is available (and
desirable).
"

So I introduced interrupt in V2. In my testcase, after smc call done,
it means firmware->smc mailbox->firmware done. Interrupt notification
from firmware->Linux, means firmware has done the operation.

When using interrupts, we could not know res.a0 as smc sync call.

Interrupts is not a must in my testcase, Florian, Andre, do you have
any comments? Should I keep interrupts in V3 or drop it as Jassi comments?

Thanks,
Peng.

> 
> > +       mbox_chan_received_data(link, (void *)res.a0);
> > +
> This is fine.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Sudeep,

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Thu, Jun 20, 2019 at 10:21:09AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > >
> > > On Mon, Jun 03, 2019 at 04:30:05PM +0800, peng.fan@nxp.com 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> 1%7
> > > Cpeng.fa
> > > >
> > >
> n%40nxp.com%7C6b37f78032e446be750e08d6f560e707%7C686ea1d3bc2b4
> > > c6fa92cd
> > > >
> > >
> 99c5c301635%7C0%7C0%7C636966193913988679&amp;sdata=UNM4MTPs
> > > brqoMqWStEy
> > > > YzzwMEWTmX7hHO3TeNEz%2BOAw%3D&amp;reserved=0
> > > >
> > > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >
> > > > V2:
> > > >  Add interrupts notification support.
> > > >
> > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > >  drivers/mailbox/Makefile                |   2 +
> > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > ++++++++++++++++++++++++++++++++
> > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > >  4 files changed, 209 insertions(+)
> > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> mode
> > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > >
> > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > --- a/drivers/mailbox/Kconfig
> > > > +++ b/drivers/mailbox/Kconfig
> > > > @@ -15,6 +15,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..fef6e38d8b98
> > > > --- /dev/null
> > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > @@ -0,0 +1,190 @@
> > > > +// 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/mailbox/arm-smc-mailbox.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> > > > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > > > +
> > > > +struct arm_smc_chan_data {
> > > > +	u32 function_id;
> > > > +	u32 flags;
> > > > +	int irq;
> > > > +};
> > > > +
> > > > +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;
> > > > +	struct arm_smccc_res res;
> > > > +	u32 function_id;
> > > > +
> > > > +	if (chan_data->function_id != UINT_MAX)
> > > > +		function_id = chan_data->function_id;
> > > > +	else
> > > > +		function_id = cmd->a0;
> > > > +
> > > > +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > > > +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > > cmd->a4,
> > > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > > +	else
> > > > +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > > cmd->a4,
> > > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > > +
> > >
> > > So how will the SMC/HVC handler in EL3/2 find which mailbox is being
> > > referred with this command ? I prefer 2nd argument to be the mailbox
> > > number.
> > You mean channel number as following?
> >
> > @@ -37,10 +38,10 @@ static int arm_smc_send_data(struct mbox_chan
> *link, void *data)
> >                 function_id = cmd->a0;
> >
> >         if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > -               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +               arm_smccc_hvc(function_id, chan_data->chan_id,
> cmd->a2, cmd->a3, cmd->a4,
> >                               cmd->a5, cmd->a6, cmd->a7, &res);
> >         else
> > -               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +               arm_smccc_smc(function_id, chan_data->chan_id,
> cmd->a2, cmd->a3, cmd->a4,
> >                               cmd->a5, cmd->a6, cmd->a7, &res);
> >
> 
> Yes something like above. There's a brief description of the same in
> latest SCMI specification though it's not related to SCMI, it more
> general note for SMC based mailbox.
> 
> "In case the doorbell is SMC/HVC based, it should follow the SMC Calling
> Convention [SMCCC] and needs to provide the identifier of the Shared
> Memory
> area that contains the payload. On return from the call, the Shared Memory
> area which contained the payload is now updated with the SCMI return
> response.
> The identifier of the Shared Memory area should be 32-bits and each
> identifier
> should denote a distinct Shared Memory area."

Thanks for the info, it make sense to pass channel id to firmware.

> 
> > Or should that be passed from firmware driver?
> >
> 
> No, we can't assume the id's in DT are 1-1 translation to mailbox ID used
> though it may be the same most of the time.

Understand.

> 
> > If not from firmware driver, just as above, I do not have a good idea which
> > should be passed to smc, from cmd->a1 to a5 or from cmd->a2 to a6.
> >
> 
> Also I found copying those registers may not be always needed and can
> be sub-optimal. May be a way to indicate that this in DT whether
> register based transfers are used or using memory. Just a thought.

"reg-transport" or "mem-transport" should help here.

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 V2 2/2] mailbox: introduce ARM SMC based mailbox

[Peng Fan]

Hi Jassi

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> ZkeRMtL8Bx
> > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  Add interrupts notification support.
> >
> >  drivers/mailbox/Kconfig                 |   7 ++
> >  drivers/mailbox/Makefile                |   2 +
> >  drivers/mailbox/arm-smc-mailbox.c       | 190
> ++++++++++++++++++++++++++++++++
> >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> >  4 files changed, 209 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > 100644 include/linux/mailbox/arm-smc-mailbox.h
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > 595542bfae85..c3bd0f1ddcd8 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,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..fef6e38d8b98
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,190 @@
> > +// 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/mailbox/arm-smc-mailbox.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > +
> IRQ bit is unused (and unnecessary IMO)
> 
> > +struct arm_smc_chan_data {
> > +       u32 function_id;
> > +       u32 flags;
> > +       int irq;
> > +};
> > +
> > +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;
> > +       struct arm_smccc_res res;
> > +       u32 function_id;
> > +
> > +       if (chan_data->function_id != UINT_MAX)
> > +               function_id = chan_data->function_id;
> > +       else
> > +               function_id = cmd->a0;
> > +
> Not sure about chan_data->function_id.  Why restrict from DT?
> 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
> values via 'a[1-7]'

Missed to reply this comment.

The firmware driver might not have func-id, such as SCMI/SCPI.
So add an optional func-id to let smc mailbox driver could
use smc SiP func id.

Thanks,
Peng.

> 
> 
> > +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +       else
> > +               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +
> > +       if (chan_data->irq)
> > +               return 0;
> > +
> This irq thing seems like oob signalling, that is, a protocol thing.
> And then it provides lesser info via chan_irq_handler (returns NULL) than
> res.a0 - which can always be ignored if not needed.
> So the irq should be implemented in the upper layer if the protocol needs it.
> 
> > +       mbox_chan_received_data(link, (void *)res.a0);
> > +
> This is fine.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Jassi
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > c6fa92cd
> > >
> > 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > ZkeRMtL8Bx
> > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > >
> > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >
> > > V2:
> > >  Add interrupts notification support.
> > >
> > >  drivers/mailbox/Kconfig                 |   7 ++
> > >  drivers/mailbox/Makefile                |   2 +
> > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > ++++++++++++++++++++++++++++++++
> > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > >  4 files changed, 209 insertions(+)
> > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > 595542bfae85..c3bd0f1ddcd8 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -15,6 +15,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..fef6e38d8b98
> > > --- /dev/null
> > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > @@ -0,0 +1,190 @@
> > > +// 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/mailbox/arm-smc-mailbox.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > +
> > IRQ bit is unused (and unnecessary IMO)
> >
> > > +struct arm_smc_chan_data {
> > > +       u32 function_id;
> > > +       u32 flags;
> > > +       int irq;
> > > +};
> > > +
> > > +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;
> > > +       struct arm_smccc_res res;
> > > +       u32 function_id;
> > > +
> > > +       if (chan_data->function_id != UINT_MAX)
> > > +               function_id = chan_data->function_id;
> > > +       else
> > > +               function_id = cmd->a0;
> > > +
> > Not sure about chan_data->function_id.  Why restrict from DT?
> > 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
> > values via 'a[1-7]'
>
> Missed to reply this comment.
>
> The firmware driver might not have func-id, such as SCMI/SCPI.
> So add an optional func-id to let smc mailbox driver could
> use smc SiP func id.
>
There is no end to conforming to protocols. Controller drivers should
be written having no particular client in mind.

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

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

[Peng Fan]

Hi All,

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > Hi Jassi
> >
> > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > >
> > > On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> 1%7
> > > Cpeng.fa
> > > >
> > >
> n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > c6fa92cd
> > > >
> > >
> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > > ZkeRMtL8Bx
> > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > > >
> > > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >
> > > > V2:
> > > >  Add interrupts notification support.
> > > >
> > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > >  drivers/mailbox/Makefile                |   2 +
> > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > ++++++++++++++++++++++++++++++++
> > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > >  4 files changed, 209 insertions(+)
> > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> mode
> > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > >
> > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > --- a/drivers/mailbox/Kconfig
> > > > +++ b/drivers/mailbox/Kconfig
> > > > @@ -15,6 +15,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..fef6e38d8b98
> > > > --- /dev/null
> > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > @@ -0,0 +1,190 @@
> > > > +// 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/mailbox/arm-smc-mailbox.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > > +
> > > IRQ bit is unused (and unnecessary IMO)
> > >
> > > > +struct arm_smc_chan_data {
> > > > +       u32 function_id;
> > > > +       u32 flags;
> > > > +       int irq;
> > > > +};
> > > > +
> > > > +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;
> > > > +       struct arm_smccc_res res;
> > > > +       u32 function_id;
> > > > +
> > > > +       if (chan_data->function_id != UINT_MAX)
> > > > +               function_id = chan_data->function_id;
> > > > +       else
> > > > +               function_id = cmd->a0;
> > > > +
> > > Not sure about chan_data->function_id.  Why restrict from DT?
> > > 'a0' is the function_id register, let the user pass func-id via the 'a0' like
> other
> > > values via 'a[1-7]'
> >
> > Missed to reply this comment.
> >
> > The firmware driver might not have func-id, such as SCMI/SCPI.
> > So add an optional func-id to let smc mailbox driver could
> > use smc SiP func id.
> >
> There is no end to conforming to protocols. Controller drivers should
> be written having no particular client in mind.

If the func-id needs be passed from user, then the chan_id suggested
by Sudeep should also be passed from user, not in mailbox driver.

Jassi, so from your point, arm_smc_send_data just send a0 - a6
to firmware, right?

Sudeep, Andre, Florian,

What's your suggestion? SCMI not support, do you have
plan to add smc transport in SCMI?

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

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

[Jassi Brar]

On Wed, Jun 26, 2019 at 8:31 AM Peng Fan <peng.fan@nxp.com> wrote:
>
>
> Hi All,
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <peng.fan@nxp.com> wrote:
> > >
> > > Hi Jassi
> > >
> > > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > > >
> > > > On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> > 1%7
> > > > Cpeng.fa
> > > > >
> > > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > > c6fa92cd
> > > > >
> > > >
> > 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > > > ZkeRMtL8Bx
> > > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > > > >
> > > > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >
> > > > > V2:
> > > > >  Add interrupts notification support.
> > > > >
> > > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > > >  drivers/mailbox/Makefile                |   2 +
> > > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > > ++++++++++++++++++++++++++++++++
> > > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > > >  4 files changed, 209 insertions(+)
> > > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> > mode
> > > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,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..fef6e38d8b98
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > > @@ -0,0 +1,190 @@
> > > > > +// 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/mailbox/arm-smc-mailbox.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +
> > > > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > > > +
> > > > IRQ bit is unused (and unnecessary IMO)
> > > >
> > > > > +struct arm_smc_chan_data {
> > > > > +       u32 function_id;
> > > > > +       u32 flags;
> > > > > +       int irq;
> > > > > +};
> > > > > +
> > > > > +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;
> > > > > +       struct arm_smccc_res res;
> > > > > +       u32 function_id;
> > > > > +
> > > > > +       if (chan_data->function_id != UINT_MAX)
> > > > > +               function_id = chan_data->function_id;
> > > > > +       else
> > > > > +               function_id = cmd->a0;
> > > > > +
> > > > Not sure about chan_data->function_id.  Why restrict from DT?
> > > > 'a0' is the function_id register, let the user pass func-id via the 'a0' like
> > other
> > > > values via 'a[1-7]'
> > >
> > > Missed to reply this comment.
> > >
> > > The firmware driver might not have func-id, such as SCMI/SCPI.
> > > So add an optional func-id to let smc mailbox driver could
> > > use smc SiP func id.
> > >
> > There is no end to conforming to protocols. Controller drivers should
> > be written having no particular client in mind.
>
> If the func-id needs be passed from user, then the chan_id suggested
> by Sudeep should also be passed from user, not in mailbox driver.
>
Isn't it already so?

> Jassi, so from your point, arm_smc_send_data just send a0 - a6
> to firmware, right?
>
Yes.

> Sudeep, Andre, Florian,
>
> What's your suggestion? SCMI not support, do you have
> plan to add smc transport in SCMI?
>
Not replying on their behalf .... but SCMI should eventually support
more than MHU. And I can't see why that matters here?

thanks.

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

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

[Florian Fainelli]

On 6/26/19 6:31 AM, Peng Fan wrote:
>>> The firmware driver might not have func-id, such as SCMI/SCPI.
>>> So add an optional func-id to let smc mailbox driver could
>>> use smc SiP func id.
>>>
>> There is no end to conforming to protocols. Controller drivers should
>> be written having no particular client in mind.
> 
> If the func-id needs be passed from user, then the chan_id suggested
> by Sudeep should also be passed from user, not in mailbox driver.
> 
> Jassi, so from your point, arm_smc_send_data just send a0 - a6
> to firmware, right?
> 
> Sudeep, Andre, Florian,
> 
> What's your suggestion? SCMI not support, do you have
> plan to add smc transport in SCMI?

On the platforms that I work with, we have taken the liberty of
implementing SCMI in our monitor firmware because the other MCU we use
for dynamic voltage and frequency scaling did not have enough memory to
support that and we still had the ability to make that firmware be
trusted enough we could give it power management responsibilities. I
would certainly feel more comfortable if the SCMI specification was
amended to indicate that the Agent could be such a software entity,
still residing on the same host CPU as the Platform(s), but if not,
that's fine.

This has lead us to implement a mailbox driver that uses a proprietary
SMC call for the P2A path ("tx" channel) and the return being done via
either that same SMC or through SGI. You can take a look at it in our
downstream tree here actually:

https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c

If we can get rid of our own driver and uses a standard SMC based
mailbox driver that supports our use case that involves interrupts (we
can always change their kind without our firmware/boot loader since FDT
is generated from that component), that would be great.
-- 
Florian

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

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

[Sudeep Holla]

On Wed, Jun 26, 2019 at 01:31:15PM +0000, Peng Fan wrote:
> 
> Hi All,
> 
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > 
> > On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <peng.fan@nxp.com> wrote:
> > >
> > > Hi Jassi
> > >
> > > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > > >
> > > > On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> > 1%7
> > > > Cpeng.fa
> > > > >
> > > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > > c6fa92cd
> > > > >
> > > >
> > 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > > > ZkeRMtL8Bx
> > > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > > > >
> > > > > Cc: Andre Przywara <andre.przywara@arm.com>
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >
> > > > > V2:
> > > > >  Add interrupts notification support.
> > > > >
> > > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > > >  drivers/mailbox/Makefile                |   2 +
> > > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > > ++++++++++++++++++++++++++++++++
> > > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > > >  4 files changed, 209 insertions(+)
> > > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> > mode
> > > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,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..fef6e38d8b98
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > > @@ -0,0 +1,190 @@
> > > > > +// 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/mailbox/arm-smc-mailbox.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +
> > > > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > > > +
> > > > IRQ bit is unused (and unnecessary IMO)
> > > >
> > > > > +struct arm_smc_chan_data {
> > > > > +       u32 function_id;
> > > > > +       u32 flags;
> > > > > +       int irq;
> > > > > +};
> > > > > +
> > > > > +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;
> > > > > +       struct arm_smccc_res res;
> > > > > +       u32 function_id;
> > > > > +
> > > > > +       if (chan_data->function_id != UINT_MAX)
> > > > > +               function_id = chan_data->function_id;
> > > > > +       else
> > > > > +               function_id = cmd->a0;
> > > > > +
> > > > Not sure about chan_data->function_id.  Why restrict from DT?
> > > > 'a0' is the function_id register, let the user pass func-id via the 'a0' like
> > other
> > > > values via 'a[1-7]'
> > >
> > > Missed to reply this comment.
> > >
> > > The firmware driver might not have func-id, such as SCMI/SCPI.
> > > So add an optional func-id to let smc mailbox driver could
> > > use smc SiP func id.
> > >
> > There is no end to conforming to protocols. Controller drivers should
> > be written having no particular client in mind.
> 
> If the func-id needs be passed from user, then the chan_id suggested
> by Sudeep should also be passed from user, not in mailbox driver.
>

Why ? I understand SMC may have 1-1 mapping from DT to channel id, but
that may not be true for other mailbox controller. The client is provided
with mbox handle in DT and the mbox APIs are used to get the controller
handle. The client just uses the same and when it calls say send_data,
controller understands which channel the handle is mapped to and
client need not have remote idea on channel id. If by user you mean
DT yes but as described as it's indirect.

> Jassi, so from your point, arm_smc_send_data just send a0 - a6
> to firmware, right?
>
> Sudeep, Andre, Florian,
>
> What's your suggestion? SCMI not support, do you have
> plan to add smc transport in SCMI?
>

I *wish* we could abstract all the transport protocol behind the mailbox
APIs and SCMI just deals with message protocol as it is a message protocol
specification.

--
Regards,
Sudeep


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

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

[André Przywara]

On 25/06/2019 08:20, Peng Fan wrote:
> Hi Jassi,
> 
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: 2019年6月21日 0:50
>> To: Peng Fan <peng.fan@nxp.com>
>> Cc: Rob Herring <robh+dt@kernel.org>; Mark Rutland
>> <mark.rutland@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; Florian
>> Fainelli <f.fainelli@gmail.com>; , Sascha Hauer <kernel@pengutronix.de>;
>> dl-linux-imx <linux-imx@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
>> festevam@gmail.com; Devicetree List <devicetree@vger.kernel.org>; Linux
>> Kernel Mailing List <linux-kernel@vger.kernel.org>;
>> linux-arm-kernel@lists.infradead.org; Andre Przywara
>> <andre.przywara@arm.com>; van.freenix@gmail.com
>> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
>>
>> On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@nxp.com> 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%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
>> c6fa92cd
>>>
>> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
>> ZkeRMtL8Bx
>>> gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
>>>
>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>
>>> V2:
>>>  Add interrupts notification support.
>>>
>>>  drivers/mailbox/Kconfig                 |   7 ++
>>>  drivers/mailbox/Makefile                |   2 +
>>>  drivers/mailbox/arm-smc-mailbox.c       | 190
>> ++++++++++++++++++++++++++++++++
>>>  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
>>>  4 files changed, 209 insertions(+)
>>>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
>>> 100644 include/linux/mailbox/arm-smc-mailbox.h
>>>
>>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
>>> 595542bfae85..c3bd0f1ddcd8 100644
>>> --- a/drivers/mailbox/Kconfig
>>> +++ b/drivers/mailbox/Kconfig
>>> @@ -15,6 +15,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..fef6e38d8b98
>>> --- /dev/null
>>> +++ b/drivers/mailbox/arm-smc-mailbox.c
>>> @@ -0,0 +1,190 @@
>>> +// 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/mailbox/arm-smc-mailbox.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
>>> +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
>>> +
>> IRQ bit is unused (and unnecessary IMO)
> 
> This will be removed in next version.
> 
>>
>>> +struct arm_smc_chan_data {
>>> +       u32 function_id;
>>> +       u32 flags;
>>> +       int irq;
>>> +};
>>> +
>>> +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;
>>> +       struct arm_smccc_res res;
>>> +       u32 function_id;
>>> +
>>> +       if (chan_data->function_id != UINT_MAX)
>>> +               function_id = chan_data->function_id;
>>> +       else
>>> +               function_id = cmd->a0;
>>> +
>> Not sure about chan_data->function_id.  Why restrict from DT?
>> 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
>> values via 'a[1-7]'
>>
>>
>>> +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
>>> +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
>> cmd->a3, cmd->a4,
>>> +                             cmd->a5, cmd->a6, cmd->a7, &res);
>>> +       else
>>> +               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
>> cmd->a3, cmd->a4,
>>> +                             cmd->a5, cmd->a6, cmd->a7, &res);
>>> +
>>> +       if (chan_data->irq)
>>> +               return 0;
>>> +
>> This irq thing seems like oob signalling, that is, a protocol thing.
>> And then it provides lesser info via chan_irq_handler (returns NULL) than
>> res.a0 - which can always be ignored if not needed.
>> So the irq should be implemented in the upper layer if the protocol needs it.
> 
> The interrupts was added here because in v1, Florian suggest
> "
> I would just put a
> provision in the binding to support an optional interrupt such that
> asynchronism gets reasonably easy to plug in when it is available (and
> desirable).
> "
> 
> So I introduced interrupt in V2. In my testcase, after smc call done,
> it means firmware->smc mailbox->firmware done. Interrupt notification
> from firmware->Linux, means firmware has done the operation.
> 
> When using interrupts, we could not know res.a0 as smc sync call.
> 
> Interrupts is not a must in my testcase, Florian, Andre, do you have
> any comments? Should I keep interrupts in V3 or drop it as Jassi comments?

The smc mailbox is by its very design a one-way channel - and it's
synchronous. I think this is all the mailbox driver should be concerned
about. The fact that there is a protocol user that would benefit from a
return channel is a separate issue.
The SCMI binding explicitly mentions *two* mailboxes, one TX, one RX, so
the return channel could be very well implemented by a separate driver.
I am wondering if we get away without a functioning return channel, at
least for a subset of SCMI functionality? Can we use some dummy driver?
Or specify another smc channel with some unhandled/ignored channel ID
for that purpose?

So I would leave the IRQ return channel out for now, unless we
desperately need it.

Cheers,
Andre.

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

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

[Florian Fainelli]

On 6/26/19 10:05 AM, André Przywara wrote:
>> So I introduced interrupt in V2. In my testcase, after smc call done,
>> it means firmware->smc mailbox->firmware done. Interrupt notification
>> from firmware->Linux, means firmware has done the operation.
>>
>> When using interrupts, we could not know res.a0 as smc sync call.
>>
>> Interrupts is not a must in my testcase, Florian, Andre, do you have
>> any comments? Should I keep interrupts in V3 or drop it as Jassi comments?
> 
> The smc mailbox is by its very design a one-way channel - and it's
> synchronous. I think this is all the mailbox driver should be concerned
> about. The fact that there is a protocol user that would benefit from a
> return channel is a separate issue.
> The SCMI binding explicitly mentions *two* mailboxes, one TX, one RX, so
> the return channel could be very well implemented by a separate driver.
> I am wondering if we get away without a functioning return channel, at
> least for a subset of SCMI functionality? Can we use some dummy driver?
> Or specify another smc channel with some unhandled/ignored channel ID
> for that purpose?
> 
> So I would leave the IRQ return channel out for now, unless we
> desperately need it.

That's fine, the initial point was specifically about the binding
already defining an optional interrupt property, but that can be removed
too if this is too much confusion or opens up the discussion beyond this
submission.
-- 
Florian

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

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

[Sudeep Holla]

On Wed, Jun 26, 2019 at 09:44:06AM -0700, Florian Fainelli wrote:
> On 6/26/19 6:31 AM, Peng Fan wrote:
> >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> >>> So add an optional func-id to let smc mailbox driver could
> >>> use smc SiP func id.
> >>>
> >> There is no end to conforming to protocols. Controller drivers should
> >> be written having no particular client in mind.
> >
> > If the func-id needs be passed from user, then the chan_id suggested
> > by Sudeep should also be passed from user, not in mailbox driver.
> >
> > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > to firmware, right?
> >
> > Sudeep, Andre, Florian,
> >
> > What's your suggestion? SCMI not support, do you have
> > plan to add smc transport in SCMI?
>
> On the platforms that I work with, we have taken the liberty of
> implementing SCMI in our monitor firmware because the other MCU we use
> for dynamic voltage and frequency scaling did not have enough memory to
> support that and we still had the ability to make that firmware be
> trusted enough we could give it power management responsibilities. I
> would certainly feel more comfortable if the SCMI specification was
> amended to indicate that the Agent could be such a software entity,
> still residing on the same host CPU as the Platform(s), but if not,
> that's fine.
>

That's completely legal and there's nothing in the specification that
prohibits. I understand it's not explicitly not mentioned and I have
been trying to get such things clarified. But since it's main focus
is on the message protocol, the clarity on transport mechanism is very
thin and there's hesitation to add more details under the impression
that it may restrict the usage.

But as I mentioned, I understand what you need there :)

> This has lead us to implement a mailbox driver that uses a proprietary
> SMC call for the P2A path ("tx" channel) and the return being done via
> either that same SMC or through SGI. You can take a look at it in our
> downstream tree here actually:
>
> https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
>

Just curious, I see it's fast call and why do you still depend on
interrupt to indicate completion of the message. Will the return from
SMC not suffice ? Sorry if I am missing something obvious.

--
Regards,
Sudeep

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

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

[Jassi Brar]

On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 6/26/19 6:31 AM, Peng Fan wrote:
> >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> >>> So add an optional func-id to let smc mailbox driver could
> >>> use smc SiP func id.
> >>>
> >> There is no end to conforming to protocols. Controller drivers should
> >> be written having no particular client in mind.
> >
> > If the func-id needs be passed from user, then the chan_id suggested
> > by Sudeep should also be passed from user, not in mailbox driver.
> >
> > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > to firmware, right?
> >
> > Sudeep, Andre, Florian,
> >
> > What's your suggestion? SCMI not support, do you have
> > plan to add smc transport in SCMI?
>
> On the platforms that I work with, we have taken the liberty of
> implementing SCMI in our monitor firmware because the other MCU we use
> for dynamic voltage and frequency scaling did not have enough memory to
> support that and we still had the ability to make that firmware be
> trusted enough we could give it power management responsibilities. I
> would certainly feel more comfortable if the SCMI specification was
> amended to indicate that the Agent could be such a software entity,
> still residing on the same host CPU as the Platform(s), but if not,
> that's fine.
>
> This has lead us to implement a mailbox driver that uses a proprietary
> SMC call for the P2A path ("tx" channel) and the return being done via
> either that same SMC or through SGI. You can take a look at it in our
> downstream tree here actually:
>
> https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
>
> If we can get rid of our own driver and uses a standard SMC based
> mailbox driver that supports our use case that involves interrupts (we
> can always change their kind without our firmware/boot loader since FDT
> is generated from that component), that would be great.
>
static irqreturn_t brcm_isr(void)
{
         mbox_chan_received_data(&chans[0], NULL);
         return IRQ_HANDLED;
}

Sorry, I fail to understand why the irq can't be moved inside the
client driver itself? There can't be more cost to it and there
definitely is no functionality lost.

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

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

[Sudeep Holla]

On Wed, Jun 26, 2019 at 01:27:41PM -0500, Jassi Brar wrote:
> On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > On 6/26/19 6:31 AM, Peng Fan wrote:
> > >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> > >>> So add an optional func-id to let smc mailbox driver could
> > >>> use smc SiP func id.
> > >>>
> > >> There is no end to conforming to protocols. Controller drivers should
> > >> be written having no particular client in mind.
> > >
> > > If the func-id needs be passed from user, then the chan_id suggested
> > > by Sudeep should also be passed from user, not in mailbox driver.
> > >
> > > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > > to firmware, right?
> > >
> > > Sudeep, Andre, Florian,
> > >
> > > What's your suggestion? SCMI not support, do you have
> > > plan to add smc transport in SCMI?
> >
> > On the platforms that I work with, we have taken the liberty of
> > implementing SCMI in our monitor firmware because the other MCU we use
> > for dynamic voltage and frequency scaling did not have enough memory to
> > support that and we still had the ability to make that firmware be
> > trusted enough we could give it power management responsibilities. I
> > would certainly feel more comfortable if the SCMI specification was
> > amended to indicate that the Agent could be such a software entity,
> > still residing on the same host CPU as the Platform(s), but if not,
> > that's fine.
> >
> > This has lead us to implement a mailbox driver that uses a proprietary
> > SMC call for the P2A path ("tx" channel) and the return being done via
> > either that same SMC or through SGI. You can take a look at it in our
> > downstream tree here actually:
> >
> > https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
> >
> > If we can get rid of our own driver and uses a standard SMC based
> > mailbox driver that supports our use case that involves interrupts (we
> > can always change their kind without our firmware/boot loader since FDT
> > is generated from that component), that would be great.
> >
> static irqreturn_t brcm_isr(void)
> {
>          mbox_chan_received_data(&chans[0], NULL);
>          return IRQ_HANDLED;
> }
> 
> Sorry, I fail to understand why the irq can't be moved inside the
> client driver itself? There can't be more cost to it and there
> definitely is no functionality lost.

What if there are multiple clients ?
And I assume you are referring to case like this where IRQ is not tied
to the mailbox IP.

--
Regards,
Sudeep


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

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

[Jassi Brar]

On Thu, Jun 27, 2019 at 4:09 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jun 26, 2019 at 01:27:41PM -0500, Jassi Brar wrote:
> > On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > >
> > > On 6/26/19 6:31 AM, Peng Fan wrote:
> > > >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> > > >>> So add an optional func-id to let smc mailbox driver could
> > > >>> use smc SiP func id.
> > > >>>
> > > >> There is no end to conforming to protocols. Controller drivers should
> > > >> be written having no particular client in mind.
> > > >
> > > > If the func-id needs be passed from user, then the chan_id suggested
> > > > by Sudeep should also be passed from user, not in mailbox driver.
> > > >
> > > > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > > > to firmware, right?
> > > >
> > > > Sudeep, Andre, Florian,
> > > >
> > > > What's your suggestion? SCMI not support, do you have
> > > > plan to add smc transport in SCMI?
> > >
> > > On the platforms that I work with, we have taken the liberty of
> > > implementing SCMI in our monitor firmware because the other MCU we use
> > > for dynamic voltage and frequency scaling did not have enough memory to
> > > support that and we still had the ability to make that firmware be
> > > trusted enough we could give it power management responsibilities. I
> > > would certainly feel more comfortable if the SCMI specification was
> > > amended to indicate that the Agent could be such a software entity,
> > > still residing on the same host CPU as the Platform(s), but if not,
> > > that's fine.
> > >
> > > This has lead us to implement a mailbox driver that uses a proprietary
> > > SMC call for the P2A path ("tx" channel) and the return being done via
> > > either that same SMC or through SGI. You can take a look at it in our
> > > downstream tree here actually:
> > >
> > > https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
> > >
> > > If we can get rid of our own driver and uses a standard SMC based
> > > mailbox driver that supports our use case that involves interrupts (we
> > > can always change their kind without our firmware/boot loader since FDT
> > > is generated from that component), that would be great.
> > >
> > static irqreturn_t brcm_isr(void)
> > {
> >          mbox_chan_received_data(&chans[0], NULL);
> >          return IRQ_HANDLED;
> > }
> >
> > Sorry, I fail to understand why the irq can't be moved inside the
> > client driver itself? There can't be more cost to it and there
> > definitely is no functionality lost.
>
> What if there are multiple clients ?
>
There is a flag IRQF_SHARED for such situations.
(good to see you considering multiple clients per channel as a legit scenario)

> And I assume you are referring to case like this where IRQ is not tied
> to the mailbox IP.
>
Yes, and that is the reason the irq should not be managed by the mailbox driver.

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

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

[Sudeep Holla]

On Thu, Jun 27, 2019 at 10:32:27AM -0500, Jassi Brar wrote:
> On Thu, Jun 27, 2019 at 4:09 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Jun 26, 2019 at 01:27:41PM -0500, Jassi Brar wrote:
> > > On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > >
> > > > On 6/26/19 6:31 AM, Peng Fan wrote:
> > > > >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> > > > >>> So add an optional func-id to let smc mailbox driver could
> > > > >>> use smc SiP func id.
> > > > >>>
> > > > >> There is no end to conforming to protocols. Controller drivers should
> > > > >> be written having no particular client in mind.
> > > > >
> > > > > If the func-id needs be passed from user, then the chan_id suggested
> > > > > by Sudeep should also be passed from user, not in mailbox driver.
> > > > >
> > > > > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > > > > to firmware, right?
> > > > >
> > > > > Sudeep, Andre, Florian,
> > > > >
> > > > > What's your suggestion? SCMI not support, do you have
> > > > > plan to add smc transport in SCMI?
> > > >
> > > > On the platforms that I work with, we have taken the liberty of
> > > > implementing SCMI in our monitor firmware because the other MCU we use
> > > > for dynamic voltage and frequency scaling did not have enough memory to
> > > > support that and we still had the ability to make that firmware be
> > > > trusted enough we could give it power management responsibilities. I
> > > > would certainly feel more comfortable if the SCMI specification was
> > > > amended to indicate that the Agent could be such a software entity,
> > > > still residing on the same host CPU as the Platform(s), but if not,
> > > > that's fine.
> > > >
> > > > This has lead us to implement a mailbox driver that uses a proprietary
> > > > SMC call for the P2A path ("tx" channel) and the return being done via
> > > > either that same SMC or through SGI. You can take a look at it in our
> > > > downstream tree here actually:
> > > >
> > > > https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
> > > >
> > > > If we can get rid of our own driver and uses a standard SMC based
> > > > mailbox driver that supports our use case that involves interrupts (we
> > > > can always change their kind without our firmware/boot loader since FDT
> > > > is generated from that component), that would be great.
> > > >
> > > static irqreturn_t brcm_isr(void)
> > > {
> > >          mbox_chan_received_data(&chans[0], NULL);
> > >          return IRQ_HANDLED;
> > > }
> > >
> > > Sorry, I fail to understand why the irq can't be moved inside the
> > > client driver itself? There can't be more cost to it and there
> > > definitely is no functionality lost.
> >
> > What if there are multiple clients ?
> >
> There is a flag IRQF_SHARED for such situations.

Indeed, we can use it.

> (good to see you considering multiple clients per channel as a legit scenario)
>

Not single channel, but single IRQ shared by multiple channels.
We can have multiple SMC based mailbox but one shared IRQ.

> > And I assume you are referring to case like this where IRQ is not tied
> > to the mailbox IP.
> >
> Yes, and that is the reason the irq should not be manageid by the mailbox driver.

Thanks for confirmation.

--
Regards,
Sudeep

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

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

[Florian Fainelli]

On 6/26/19 10:09 AM, Sudeep Holla wrote:
> On Wed, Jun 26, 2019 at 09:44:06AM -0700, Florian Fainelli wrote:
>> On 6/26/19 6:31 AM, Peng Fan wrote:
>>>>> The firmware driver might not have func-id, such as SCMI/SCPI.
>>>>> So add an optional func-id to let smc mailbox driver could
>>>>> use smc SiP func id.
>>>>>
>>>> There is no end to conforming to protocols. Controller drivers should
>>>> be written having no particular client in mind.
>>>
>>> If the func-id needs be passed from user, then the chan_id suggested
>>> by Sudeep should also be passed from user, not in mailbox driver.
>>>
>>> Jassi, so from your point, arm_smc_send_data just send a0 - a6
>>> to firmware, right?
>>>
>>> Sudeep, Andre, Florian,
>>>
>>> What's your suggestion? SCMI not support, do you have
>>> plan to add smc transport in SCMI?
>>
>> On the platforms that I work with, we have taken the liberty of
>> implementing SCMI in our monitor firmware because the other MCU we use
>> for dynamic voltage and frequency scaling did not have enough memory to
>> support that and we still had the ability to make that firmware be
>> trusted enough we could give it power management responsibilities. I
>> would certainly feel more comfortable if the SCMI specification was
>> amended to indicate that the Agent could be such a software entity,
>> still residing on the same host CPU as the Platform(s), but if not,
>> that's fine.
>>
> 
> That's completely legal and there's nothing in the specification that
> prohibits. I understand it's not explicitly not mentioned and I have
> been trying to get such things clarified. But since it's main focus
> is on the message protocol, the clarity on transport mechanism is very
> thin and there's hesitation to add more details under the impression
> that it may restrict the usage.
> 
> But as I mentioned, I understand what you need there :)
> 
>> This has lead us to implement a mailbox driver that uses a proprietary
>> SMC call for the P2A path ("tx" channel) and the return being done via
>> either that same SMC or through SGI. You can take a look at it in our
>> downstream tree here actually:
>>
>> https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
>>
> 
> Just curious, I see it's fast call and why do you still depend on
> interrupt to indicate completion of the message. Will the return from
> SMC not suffice ? Sorry if I am missing something obvious.

It is currently used for synchronous delayed responses where the SMC
call returns early, but the operation is carried out asynchronously by
e.g: the MCU that does voltage scaling a few milliseconds later. We'd
rather not block the caller for too long and that's where it stems from.
-- 
Florian



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

Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Rob Herring]

On Mon, Jun 03, 2019 at 04:30:04PM +0800, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The ARM SMC 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>
> ---
> 
> V2:
> Introduce interrupts as a property.
> 
> V1:
> arm,func-ids is still kept as an optional property, because there is no
> defined SMC funciton id passed from SCMI. So in my test, I still use
> arm,func-ids for ARM SIP service.
> 
>  .../devicetree/bindings/mailbox/arm-smc.txt        | 101 +++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> new file mode 100644
> index 000000000000..401887118c09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> @@ -0,0 +1,101 @@
> +ARM SMC Mailbox Interface
> +=========================
> +
> +This mailbox uses the ARM smc (secure monitor 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[1].
> +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.
> +
> +Mailbox Device Node:
> +====================
> +
> +This node is expected to be a child of the /firmware node.
> +
> +Required properties:
> +--------------------
> +- compatible:		Shall be "arm,smc-mbox"
> +- #mbox-cells		Shall be 1 - the index of the channel needed.
> +- arm,num-chans		The number of channels supported.
> +- method:		A string, either:
> +			"hvc": if the driver shall use an HVC call, or
> +			"smc": if the driver shall use an SMC call.
> +
> +Optional properties:
> +- arm,func-ids		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.
> +- interrupts		SPI interrupts may be listed for notification,
> +			each channel should use a dedicated interrupt
> +			line.
> +
> +Example:
> +--------
> +
> +	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>;
> +		};
> +	};
> +
> +	smc_mbox: mailbox {

This should be a child of 'firmware' node at least and really a child of 
the firmware component that implements the feature.

> +		#mbox-cells = <1>;
> +		compatible = "arm,smc-mbox";
> +		method = "smc";
> +		arm,num-chans = <0x2>;
> +		/* Optional */
> +		arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> +	};
> +
> +	firmware {
> +		scmi {
> +			compatible = "arm,scmi";
> +			mboxes = <&mailbox 0 &mailbox 1>;
> +			mbox-names = "tx", "rx";
> +			shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
> +		};
> +	};
> +
> +
> +[1]
> +http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0028a/index.html
> -- 
> 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 V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Peng Fan]

Hi Rob,

> Subject: Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC
> mailbox
> 
> On Mon, Jun 03, 2019 at 04:30:04PM +0800, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The ARM SMC 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>
> > ---
> >
> > V2:
> > Introduce interrupts as a property.
> >
> > V1:
> > arm,func-ids is still kept as an optional property, because there is
> > no defined SMC funciton id passed from SCMI. So in my test, I still
> > use arm,func-ids for ARM SIP service.
> >
> >  .../devicetree/bindings/mailbox/arm-smc.txt        | 101
> +++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/mailbox/arm-smc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > new file mode 100644
> > index 000000000000..401887118c09
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > @@ -0,0 +1,101 @@
> > +ARM SMC Mailbox Interface
> > +=========================
> > +
> > +This mailbox uses the ARM smc (secure monitor 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[1].
> > +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.
> > +
> > +Mailbox Device Node:
> > +====================
> > +
> > +This node is expected to be a child of the /firmware node.
> > +
> > +Required properties:
> > +--------------------
> > +- compatible:		Shall be "arm,smc-mbox"
> > +- #mbox-cells		Shall be 1 - the index of the channel needed.
> > +- arm,num-chans		The number of channels supported.
> > +- method:		A string, either:
> > +			"hvc": if the driver shall use an HVC call, or
> > +			"smc": if the driver shall use an SMC call.
> > +
> > +Optional properties:
> > +- arm,func-ids		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.
> > +- interrupts		SPI interrupts may be listed for notification,
> > +			each channel should use a dedicated interrupt
> > +			line.
> > +
> > +Example:
> > +--------
> > +
> > +	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>;
> > +		};
> > +	};
> > +
> > +	smc_mbox: mailbox {
> 
> This should be a child of 'firmware' node at least and really a child of the
> firmware component that implements the feature.

I checked other mbox driver, including the mbox used by ti sci, mbox used by
i.MX8QXP. both mbox dts node not a child a firmware node,
I am not sure why put mbox node into a child a firmware node here.

Thanks,
Peng.

> 
> > +		#mbox-cells = <1>;
> > +		compatible = "arm,smc-mbox";
> > +		method = "smc";
> > +		arm,num-chans = <0x2>;
> > +		/* Optional */
> > +		arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > +	};
> > +
> > +	firmware {
> > +		scmi {
> > +			compatible = "arm,scmi";
> > +			mboxes = <&mailbox 0 &mailbox 1>;
> > +			mbox-names = "tx", "rx";
> > +			shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
> > +		};
> > +	};
> > +
> > +
> > +[1]
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> >
> +center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.den002
> 8a%2
> >
> +Findex.html&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7Cd8cf8b81b4f
> b49be5
> >
> +97c08d703f26576%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7
> C63698221
> >
> +1931902513&amp;sdata=RtHkNN07b%2FuzdJkiu0QujeJ6czrcwOwEI6Y6JW
> VpPkY%3D
> > +&amp;reserved=0
> > --
> > 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 V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox

[Rob Herring]

On Mon, Jul 8, 2019 at 7:40 PM Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Rob,
>
> > Subject: Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC
> > mailbox
> >
> > On Mon, Jun 03, 2019 at 04:30:04PM +0800, peng.fan@nxp.com wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The ARM SMC 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>
> > > ---
> > >
> > > V2:
> > > Introduce interrupts as a property.
> > >
> > > V1:
> > > arm,func-ids is still kept as an optional property, because there is
> > > no defined SMC funciton id passed from SCMI. So in my test, I still
> > > use arm,func-ids for ARM SIP service.
> > >
> > >  .../devicetree/bindings/mailbox/arm-smc.txt        | 101
> > +++++++++++++++++++++
> > >  1 file changed, 101 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > new file mode 100644
> > > index 000000000000..401887118c09
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> > > @@ -0,0 +1,101 @@
> > > +ARM SMC Mailbox Interface
> > > +=========================
> > > +
> > > +This mailbox uses the ARM smc (secure monitor 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[1].
> > > +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.
> > > +
> > > +Mailbox Device Node:
> > > +====================
> > > +
> > > +This node is expected to be a child of the /firmware node.
> > > +
> > > +Required properties:
> > > +--------------------
> > > +- compatible:              Shall be "arm,smc-mbox"
> > > +- #mbox-cells              Shall be 1 - the index of the channel needed.
> > > +- arm,num-chans            The number of channels supported.
> > > +- method:          A string, either:
> > > +                   "hvc": if the driver shall use an HVC call, or
> > > +                   "smc": if the driver shall use an SMC call.
> > > +
> > > +Optional properties:
> > > +- arm,func-ids             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.
> > > +- interrupts               SPI interrupts may be listed for notification,
> > > +                   each channel should use a dedicated interrupt
> > > +                   line.
> > > +
> > > +Example:
> > > +--------
> > > +
> > > +   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>;
> > > +           };
> > > +   };
> > > +
> > > +   smc_mbox: mailbox {
> >
> > This should be a child of 'firmware' node at least and really a child of the
> > firmware component that implements the feature.
>
> I checked other mbox driver, including the mbox used by ti sci, mbox used by
> i.MX8QXP. both mbox dts node not a child a firmware node,

Because those are actual h/w blocks and not implemented in firmware calls?

> I am not sure why put mbox node into a child a firmware node here.

If it is an interface provided by firmware, then it goes under /firmware.

Rob

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