[PATCH 0/8] mailbox: arm/arm64: introduce smc triggered mailbox

[PATCH 0/8] mailbox: arm/arm64: introduce smc triggered mailbox

[Andre Przywara]

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 SCPI protocol to signal a
request for some action to be taken by the management processor.
And while it seems useful to have a dedicated management core to provide
those services offered via the SCPI protocol, a separate and independent
execution unit is not always required, for instance for just setting up
a clock or enabling a device power supply. Those services could be as well
provided by firmware code running on the very same application processor
cores as the OS, using the ARM TrustZone mechanism to protect and abstract
those services. And while the existing SCPI 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 SCPI requests.
The actual requests are still communicated using the standard SCPI way of
shared memory regions, but a dedicated mailbox hardware IP can be replaced via
this new driver, which not only serves as a trigger for the request, but also
transfers execution to the firmware code in a safe and architected way.

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 introduces the actual mailbox driver, patch 2 contains the respective
device tree binding documentation, patch 3 enabled the driver in Kconfig.

The remaining patches demonstrate usage of this feature to drive SCPI services
implemented as part of the ARM Trusted Firmware implementation used for
AArch64 based Allwinner SoCs, the Allwinner A64 in this example.
It allows to provide DVFS services, sensors support, device power domains
and potentially other services like clocks or regulators.
This allows to abstract those features in firmware, without the need to
implement explicit Linux support for each variant of some SoC design.
Those DT changes are not necessarily meant to be merged at this point.
I started implementing the firmware side of those services and put a WIP
branch on my ATF Github repo [1]. With this branch and these patches here
you get DVFS and temperature sensor support for the A64, just with this
driver and the generic SCPI support.

Please note that this driver just provides a generic mailbox mechanism,
though this is synchronous and one-way only (triggered by the OS only,
without providing an asynchronous way of triggering request from the
firmware).
And while providing SCPI 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.

Please have a look and comment whether this sounds like a useful addition
to the kernel.

Cheers,
Andre.

[1] https://github.com/apritzel/arm-trusted-firmware/commits/allwinner-scpi-wip

Andre Przywara (8):
  mailbox: introduce ARM SMC based mailbox
  dt-bindings: mailbox: add binding doc for the ARM SMC mailbox
  mailbox: Kconfig: enable ARM SMC mailbox on 64-bit Allwinner SoCs
  arm64: dts: allwinner: a64: add SCPI support
  arm64: dts: allwinner: a64: add SCPI DVFS nodes
  arm64: dts: allwinner: a64: add SCPI sensors support
  arm64: dts: allwinner: a64: add SCPI power domain support
  arm64: dts: allwinner: a64: add (unused) MMC clock node

 .../devicetree/bindings/mailbox/arm-smc.txt        |  61 ++++++++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  63 ++++++++
 drivers/mailbox/Kconfig                            |   9 ++
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/arm-smc-mailbox.c                  | 172 +++++++++++++++++++++
 5 files changed, 307 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c

-- 
2.9.0

[PATCH 1/8] mailbox: introduce ARM SMC based mailbox

[Andre Przywara]

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.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/arm-smc-mailbox.c | 172 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c5731e5..5664b7f 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -170,4 +170,12 @@ config BCM_FLEXRM_MBOX
 	  Mailbox implementation of the Broadcom FlexRM ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom FlexRM.
+
+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.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index d54e412..8ec6869 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -35,3 +35,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
 obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_ARM_SMC_MBOX)	+= arm-smc-mailbox.o
diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
new file mode 100644
index 0000000..578aed2
--- /dev/null
+++ b/drivers/mailbox/arm-smc-mailbox.c
@@ -0,0 +1,172 @@
+/*
+ *  Copyright (C) 2016,2017 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This device provides a mechanism for emulating a mailbox by using
+ * smc calls, allowing a "mailbox" consumer to sit in firmware running
+ * on the same core.
+ */
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/arm-smccc.h>
+
+#define ARM_SMC_MBOX_SMC		(0 << 0)
+#define ARM_SMC_MBOX_HVC		(1 << 0)
+#define ARM_SMC_MBOX_METHOD_MASK	(1 << 0)
+
+struct arm_smc_chan_data {
+	u32 function_id;
+	u32 flags;
+};
+
+static int arm_smc_send_data(struct mbox_chan *link, void *data)
+{
+	struct arm_smc_chan_data *chan_data = link->con_priv;
+	u32 function_id = chan_data->function_id;
+	struct arm_smccc_res res;
+	u32 msg = *(u32 *)data;
+
+	if ((chan_data->flags & ARM_SMC_MBOX_METHOD_MASK) == ARM_SMC_MBOX_SMC)
+		arm_smccc_smc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
+	else
+		arm_smccc_hvc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
+
+	mbox_chan_received_data(link, (void *)res.a0);
+
+	return 0;
+}
+
+static int arm_smc_startup(struct mbox_chan *link)
+{
+	return 0;
+}
+
+static void arm_smc_shutdown(struct mbox_chan *link)
+{
+}
+
+/* This mailbox is synchronous, so we are always done. */
+static bool arm_smc_last_tx_done(struct mbox_chan *link)
+{
+	return true;
+}
+
+static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
+	.send_data	= arm_smc_send_data,
+	.startup	= arm_smc_startup,
+	.shutdown	= arm_smc_shutdown,
+	.last_tx_done	= arm_smc_last_tx_done
+};
+
+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 = 0, i;
+
+	ret = of_property_count_elems_of_size(dev->of_node, "arm,smc-func-ids",
+					      sizeof(u32));
+	if (ret < 0)
+		return ret;
+
+	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 = ret;
+	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,smc-func-ids", i,
+						 &function_id);
+		if (ret)
+			return ret;
+
+		chan_data[i].function_id = function_id;
+		if (use_hvc)
+			chan_data[i].flags |= ARM_SMC_MBOX_HVC;
+		mbox->chans[i].con_priv = &chan_data[i];
+	}
+
+	mbox->txdone_poll = true;
+	mbox->txdone_irq = false;
+	mbox->txpoll_period = 1;
+	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");
-- 
2.9.0

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

[Andre Przywara]

Add binding documentation for the generic ARM SMC mailbox.
This is not describing hardware, but a firmware interface.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../devicetree/bindings/mailbox/arm-smc.txt        | 61 ++++++++++++++++++++++
 1 file changed, 61 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 0000000..90c5926
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
@@ -0,0 +1,61 @@
+ARM SMC Mailbox Driver
+======================
+
+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. 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.
+
+One use case of this mailbox is the SCP 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.
+The communication follows the ARM SMC calling convention[1].
+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:
+====================
+
+Required properties:
+--------------------
+- compatible:		Shall be "arm,smc-mbox"
+- #mbox-cells		Shall be 1 - the index of the channel needed.
+- arm,smc-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.
+
+Optional properties:
+--------------------
+- method:		A string, either:
+			"hvc": if the driver shall use an HVC call, or
+			"smc": if the driver shall use an SMC call
+			If omitted, defaults to an SMC call.
+
+Example:
+--------
+
+	mailbox: smc_mbox {
+		#mbox-cells = <1>;
+		compatible = "arm,smc-mbox";
+		identifiers = <0x82000001>, <0x82000002>;
+	};
+
+	scpi {
+		compatible = "arm,scpi";
+		mboxes = <&mailbox 0>;
+		shmem = <&cpu_scp_shmem>;
+	};
+
+
+[1]
+http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0028a/index.html
-- 
2.9.0

[PATCH 3/8] mailbox: Kconfig: enable ARM SMC mailbox on 64-bit Allwinner SoCs

[Andre Przywara]

For 64-bit Allwinner SoCs there exist firmware implementations which
provide SCPI controlled handlers for DVFS, sensors and power domains.
To allow usage of this features, enable the required SMC mailbox when
Allwinner SoCs are supported by the kernel.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mailbox/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 5664b7f..6d6e8bb 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -174,6 +174,7 @@ config BCM_FLEXRM_MBOX
 config ARM_SMC_MBOX
 	tristate "Generic ARM smc mailbox"
 	depends on OF && HAVE_ARM_SMCCC
+	default y if ARM64 && ARCH_SUNXI
 	help
 	  Generic mailbox driver which uses ARM smc calls to call into
 	  firmware for triggering mailboxes.
-- 
2.9.0

[PATCH 4/8] arm64: dts: allwinner: a64: add SCPI support

[Andre Przywara]

This adds support for the SCPI protocol using an SMC mailbox and some
shared memory in SRAM.
The SCPI provider is implemented in the ARM Trusted Firmware layer
(running in EL3 on the application processor cores), triggered by an smc
call.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 9d00622..ef6f10e 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -124,6 +124,32 @@
 			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 	};
 
+	mailbox: mbox@0 {
+		compatible = "arm,smc-mbox";
+		#mbox-cells = <1>;
+		arm,smc-func-ids = <0x82000001>;
+	};
+
+	sram: sram@10000{
+		compatible = "mmio-sram";
+		reg = <0x10000 0x8000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0x10000 0x8000>;
+
+		cpu_scp_mem: scp-shmem@7e00 {
+			compatible = "mmio-sram";
+			reg = <0x7e00 0x200>;
+		};
+	};
+
+	scpi {
+		compatible = "arm,scpi";
+		mboxes = <&mailbox 0>;
+		shmem = <&cpu_scp_mem>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.9.0

[PATCH 5/8] arm64: dts: allwinner: a64: add SCPI DVFS nodes

[Andre Przywara]

One functionality provided by the SCPI handler is frequency scaling,
which allows to switch the one CPU cluster between several operating
points, each specifying a matching frequency and CPU voltage.
The actual table is specified in firmware and can be queried by Linux
using standardised SCPI calls.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index ef6f10e..58c3675 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -61,6 +61,7 @@
 			device_type = "cpu";
 			reg = <0>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu1: cpu@1 {
@@ -68,6 +69,7 @@
 			device_type = "cpu";
 			reg = <1>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu2: cpu@2 {
@@ -75,6 +77,7 @@
 			device_type = "cpu";
 			reg = <2>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu3: cpu@3 {
@@ -82,6 +85,7 @@
 			device_type = "cpu";
 			reg = <3>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 	};
 
@@ -148,6 +152,17 @@
 		compatible = "arm,scpi";
 		mboxes = <&mailbox 0>;
 		shmem = <&cpu_scp_mem>;
+
+		scpi-clocks {
+			compatible = "arm,scpi-clocks";
+
+			scpi_dvfs: scpi_dvfs_clocks {
+				compatible = "arm,scpi-dvfs-clocks";
+				#clock-cells = <1>;
+				clock-indices = <0>;
+				clock-output-names = "cpu_clk";
+			};
+		};
 	};
 
 	soc {
-- 
2.9.0

[PATCH 6/8] arm64: dts: allwinner: a64: add SCPI sensors support

[Andre Przywara]

The SCPI protocol allows various sensors to be exposed to the OS. The
list of supported sensors (and their kind) is provided by the SCPI
provider, which is in ARM Trusted Firmware. The current implementation
exports the temperature sensors, for instance.
Since the temperature sensor requires a clock to be running, we set
a fixed clock rate for this particular clock to prevent the Linux driver
from turning it off.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 58c3675..7cb1b04 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -163,6 +163,11 @@
 				clock-output-names = "cpu_clk";
 			};
 		};
+
+		scpi_sensors0: sensors {
+			compatible = "arm,scpi-sensors";
+			#thermal-sensor-cells = <1>;
+		};
 	};
 
 	soc {
@@ -307,6 +312,8 @@
 			clock-names = "hosc", "losc";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
+			assigned-clocks = <&ccu CLK_THS>;
+			assigned-clock-rates = <4000000>;
 		};
 
 		pio: pinctrl@1c20800 {
-- 
2.9.0

[PATCH 7/8] arm64: dts: allwinner: a64: add SCPI power domain support

[Andre Przywara]

The SCPI protocol allows controlling device power domains, which
abstracts the various ways of providing voltage to devices like Ethernet
PHYs or on-board WiFi chips.
Provide the power domain provider DT node, which can be referenced by
a power domain consumer device.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 7cb1b04..30cad44 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -168,6 +168,12 @@
 			compatible = "arm,scpi-sensors";
 			#thermal-sensor-cells = <1>;
 		};
+
+		scpi_devpd: scpi-power-domains {
+			compatible = "arm,scpi-power-domains";
+			num-domains = <2>;
+			#power-domain-cells = <1>;
+		};
 	};
 
 	soc {
-- 
2.9.0

[PATCH 8/8] arm64: dts: allwinner: a64: add (unused) MMC clock node

[Andre Przywara]

More of a demo than to be really useful as it this adds a clock producer
node for the MMC clock.
This can be used instead of the existing clock driver for driving a clock.
Use cases include providing clock control by hypervisors or driving new
SoCs which don't have an appropriate clock driver in the kernel.
The number here is made explicitly compatible with the in-kernel CCF
numbering, so any consumer can be switched over by just exchaning the
phandle.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 30cad44..6949a70 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -162,6 +162,15 @@
 				clock-indices = <0>;
 				clock-output-names = "cpu_clk";
 			};
+
+			scpi_clk: scpi_clocks {
+				compatible = "arm,scpi-variable-clocks";
+				#clock-cells = <1>;
+				clock-indices = <75>, <76>,
+						<77>;
+				clock-output-names = "mmc0_clk", "mmc1_clk",
+						     "mmc2_clk";
+			};
 		};
 
 		scpi_sensors0: sensors {
-- 
2.9.0

Re: [PATCH 0/8] mailbox: arm/arm64: introduce smc triggered mailbox

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

Hi,

On Fri, Jun 30, 2017 at 10:56:00AM +0100, Andre Przywara wrote:
> The remaining patches demonstrate usage of this feature to drive SCPI services
> implemented as part of the ARM Trusted Firmware implementation used for
> AArch64 based Allwinner SoCs, the Allwinner A64 in this example.
> It allows to provide DVFS services, sensors support, device power domains
> and potentially other services like clocks or regulators.
> This allows to abstract those features in firmware, without the need to
> implement explicit Linux support for each variant of some SoC design.
> Those DT changes are not necessarily meant to be merged at this point.
> I started implementing the firmware side of those services and put a WIP
> branch on my ATF Github repo [1]. With this branch and these patches here
> you get DVFS and temperature sensor support for the A64, just with this
> driver and the generic SCPI support.

I would go even further, and say that these changes should be done by
the bootloader itself once it installed the proper monitor.

These patches represent not a state of the hardware itself, but the
state the bootloader let the hardware in, and that state will change
from one bootloader to the other, and one version to the other
(obviously). This is already what we do for the other things the
bootloader initializes (like simplefb, or PSCI). It just feels natural
to do the same thing here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/8] mailbox: arm/arm64: introduce smc triggered mailbox

[Andre Przywara]

Hi,

thanks for having a look!

On 30/06/17 13:25, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Jun 30, 2017 at 10:56:00AM +0100, Andre Przywara wrote:
>> The remaining patches demonstrate usage of this feature to drive SCPI services
>> implemented as part of the ARM Trusted Firmware implementation used for
>> AArch64 based Allwinner SoCs, the Allwinner A64 in this example.
>> It allows to provide DVFS services, sensors support, device power domains
>> and potentially other services like clocks or regulators.
>> This allows to abstract those features in firmware, without the need to
>> implement explicit Linux support for each variant of some SoC design.
>> Those DT changes are not necessarily meant to be merged at this point.
>> I started implementing the firmware side of those services and put a WIP
>> branch on my ATF Github repo [1]. With this branch and these patches here
>> you get DVFS and temperature sensor support for the A64, just with this
>> driver and the generic SCPI support.
> 
> I would go even further, and say that these changes should be done by
> the bootloader itself once it installed the proper monitor.
> 
> These patches represent not a state of the hardware itself, but the
> state the bootloader let the hardware in, and that state will change
> from one bootloader to the other, and one version to the other
> (obviously). This is already what we do for the other things the
> bootloader initializes (like simplefb, or PSCI). It just feels natural
> to do the same thing here.

Yes, indeed that was my thinking as well. I just put those DT changes in
here to demonstrate how it would look like.

Technically ATF (as the provider for the SCPI services) should do the DT
change. That would also assure that those changes are always in sync
with what's implemented. Mainline ATF seems to include libfdt now, but I
haven't checked in detail how much this covers and how easy it is to
use. Also that would mean that the base DT has to be somewhere
accessible by the ATF. So while this is eventually feasible to do (since
the dtb is loaded by U-Boot's SPL, for instance), it's just not trivial
or already implemented, that's why I would revert to provide some static
DT and match that up in some firmware image for the time being.

So indeed this makes those DT change not subject to Linux' concerns -
and might trigger more discussions on the future of DTs in the kernel ;-)

Cheers,
Andre.

Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox

[Jassi Brar]

On Fri, Jun 30, 2017 at 3:26 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> 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.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/mailbox/Kconfig           |   8 ++
>  drivers/mailbox/Makefile          |   2 +
>  drivers/mailbox/arm-smc-mailbox.c | 172 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 182 insertions(+)
>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index c5731e5..5664b7f 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -170,4 +170,12 @@ config BCM_FLEXRM_MBOX
>           Mailbox implementation of the Broadcom FlexRM ring manager,
>           which provides access to various offload engines on Broadcom
>           SoCs. Say Y here if you want to use the Broadcom FlexRM.
> +
> +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.
> +
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index d54e412..8ec6869 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -35,3 +35,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX) += bcm-flexrm-mailbox.o
>  obj-$(CONFIG_QCOM_APCS_IPC)    += qcom-apcs-ipc-mailbox.o
>
>  obj-$(CONFIG_TEGRA_HSP_MBOX)   += tegra-hsp.o
> +
> +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
> new file mode 100644
> index 0000000..578aed2
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,172 @@
> +/*
> + *  Copyright (C) 2016,2017 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This device provides a mechanism for emulating a mailbox by using
> + * smc calls, allowing a "mailbox" consumer to sit in firmware running
> + * on the same core.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/arm-smccc.h>
> +
Please have relook at what headers are really needed.

> +#define ARM_SMC_MBOX_SMC               (0 << 0)
> +#define ARM_SMC_MBOX_HVC               (1 << 0)
> +#define ARM_SMC_MBOX_METHOD_MASK       (1 << 0)
> +
Maybe have only
   #define ARM_SMC_MBOX_HVC    BIT(0)

> +struct arm_smc_chan_data {
> +       u32 function_id;
> +       u32 flags;
> +};
> +
> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> +{
> +       struct arm_smc_chan_data *chan_data = link->con_priv;
> +       u32 function_id = chan_data->function_id;
> +       struct arm_smccc_res res;
> +       u32 msg = *(u32 *)data;
> +
> +       if ((chan_data->flags & ARM_SMC_MBOX_METHOD_MASK) == ARM_SMC_MBOX_SMC)
> +               arm_smccc_smc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
> +       else
> +               arm_smccc_hvc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
> +
       if (chan_data->flags & ARM_SMC_MBOX_HVC)
               arm_smccc_hvc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
       else
               arm_smccc_smc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);

is simpler.

> +       mbox_chan_received_data(link, (void *)res.a0);
> +
Or you can update the 'data' with value from 'a0' ?

> +       return 0;
> +}
> +
> +static int arm_smc_startup(struct mbox_chan *link)
> +{
> +       return 0;
> +}
> +
> +static void arm_smc_shutdown(struct mbox_chan *link)
> +{
> +}
> +
startup and shutdown can be omitted now.

> +/* This mailbox is synchronous, so we are always done. */
> +static bool arm_smc_last_tx_done(struct mbox_chan *link)
> +{
> +       return true;
> +}
> +
> +static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
> +       .send_data      = arm_smc_send_data,
> +       .startup        = arm_smc_startup,
> +       .shutdown       = arm_smc_shutdown,
> +       .last_tx_done   = arm_smc_last_tx_done
> +};
> +
> +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 = 0, i;
> +
No need to initialise 'ret'

> +       ret = of_property_count_elems_of_size(dev->of_node, "arm,smc-func-ids",
> +                                             sizeof(u32));
> +       if (ret < 0)
> +               return ret;
> +
> +       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 = ret;
> +       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,smc-func-ids", i,
> +                                                &function_id);
> +               if (ret)
> +                       return ret;
> +
> +               chan_data[i].function_id = function_id;
> +               if (use_hvc)
> +                       chan_data[i].flags |= ARM_SMC_MBOX_HVC;
> +               mbox->chans[i].con_priv = &chan_data[i];
> +       }
> +
> +       mbox->txdone_poll = true;
> +       mbox->txdone_irq = false;
> +       mbox->txpoll_period = 1;
> +       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");
> --
> 2.9.0
>

Re: [PATCH 0/8] mailbox: arm/arm64: introduce smc triggered mailbox

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2847 bytes --]

On Fri, Jun 30, 2017 at 01:56:04PM +0100, Andre Przywara wrote:
> Hi,
> 
> thanks for having a look!
> 
> On 30/06/17 13:25, Maxime Ripard wrote:
> > Hi,
> > 
> > On Fri, Jun 30, 2017 at 10:56:00AM +0100, Andre Przywara wrote:
> >> The remaining patches demonstrate usage of this feature to drive SCPI services
> >> implemented as part of the ARM Trusted Firmware implementation used for
> >> AArch64 based Allwinner SoCs, the Allwinner A64 in this example.
> >> It allows to provide DVFS services, sensors support, device power domains
> >> and potentially other services like clocks or regulators.
> >> This allows to abstract those features in firmware, without the need to
> >> implement explicit Linux support for each variant of some SoC design.
> >> Those DT changes are not necessarily meant to be merged at this point.
> >> I started implementing the firmware side of those services and put a WIP
> >> branch on my ATF Github repo [1]. With this branch and these patches here
> >> you get DVFS and temperature sensor support for the A64, just with this
> >> driver and the generic SCPI support.
> > 
> > I would go even further, and say that these changes should be done by
> > the bootloader itself once it installed the proper monitor.
> > 
> > These patches represent not a state of the hardware itself, but the
> > state the bootloader let the hardware in, and that state will change
> > from one bootloader to the other, and one version to the other
> > (obviously). This is already what we do for the other things the
> > bootloader initializes (like simplefb, or PSCI). It just feels natural
> > to do the same thing here.
> 
> Yes, indeed that was my thinking as well. I just put those DT changes in
> here to demonstrate how it would look like.
> 
> Technically ATF (as the provider for the SCPI services) should do the DT
> change. That would also assure that those changes are always in sync
> with what's implemented. Mainline ATF seems to include libfdt now, but I
> haven't checked in detail how much this covers and how easy it is to
> use.

Even with just the libfdt's DT manipulation functions, this should be
quite easy. The steps needed would be:
  - Retrieve the phandle of the CCU node
  - Delete the CCU node (or mark it disabled, and remove its phandle
    property)
  - Add the SCPI nodes with the original's CCU phandle in your
    variable clocks subnode

The last part especially might be a bit painful, especially the
generation of the indices and names.

You also have the option of using an overlay and applying it if your
libfdt is recent enough. That way, you'll just need to care about
keeping the phandle in your code, but that's all.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Rob Herring]

On Fri, Jun 30, 2017 at 10:56:02AM +0100, Andre Przywara wrote:
> Add binding documentation for the generic ARM SMC mailbox.
> This is not describing hardware, but a firmware interface.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../devicetree/bindings/mailbox/arm-smc.txt        | 61 ++++++++++++++++++++++
>  1 file changed, 61 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 0000000..90c5926
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> @@ -0,0 +1,61 @@
> +ARM SMC Mailbox Driver

s/Driver/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. 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.
> +
> +One use case of this mailbox is the SCP 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.
> +The communication follows the ARM SMC calling convention[1].
> +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:
> +====================

Please state this should be a child of /firmware node.

> +
> +Required properties:
> +--------------------
> +- compatible:		Shall be "arm,smc-mbox"
> +- #mbox-cells		Shall be 1 - the index of the channel needed.
> +- arm,smc-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.
> +
> +Optional properties:
> +--------------------
> +- method:		A string, either:
> +			"hvc": if the driver shall use an HVC call, or
> +			"smc": if the driver shall use an SMC call
> +			If omitted, defaults to an SMC call.
> +
> +Example:
> +--------
> +
> +	mailbox: smc_mbox {

swap these. mailbox for the node name.

> +		#mbox-cells = <1>;
> +		compatible = "arm,smc-mbox";
> +		identifiers = <0x82000001>, <0x82000002>;
> +	};
> +
> +	scpi {
> +		compatible = "arm,scpi";
> +		mboxes = <&mailbox 0>;
> +		shmem = <&cpu_scp_shmem>;
> +	};
> +
> +
> +[1]
> +http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0028a/index.html
> -- 
> 2.9.0
> 

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

[Mark Rutland]

Hi Andre,

As a nit, please post bindings before drivers, as per
Documentation/devicetree/bindings/submitting-patches.txt.

On Fri, Jun 30, 2017 at 10:56:02AM +0100, Andre Przywara wrote:
> Add binding documentation for the generic ARM SMC mailbox.
> This is not describing hardware, but a firmware interface.

Is this following some standard, or is this something that you have
invented? If the latter, why are we inventing a new standard? |How does
this relate to SCPI and/or SCMI? 

What exactly is "the generic ARM SMC mailbox"?

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../devicetree/bindings/mailbox/arm-smc.txt        | 61 ++++++++++++++++++++++
>  1 file changed, 61 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 0000000..90c5926
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
> @@ -0,0 +1,61 @@
> +ARM SMC Mailbox Driver

Bindings do not document drivers. As Rob said, s/Driver/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. 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.

... where those values are what specifically, under what circumstances?

> +One use case of this mailbox is the SCP 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.
> +The communication follows the ARM SMC calling convention[1].
> +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:
> +====================
> +
> +Required properties:
> +--------------------
> +- compatible:		Shall be "arm,smc-mbox"
> +- #mbox-cells		Shall be 1 - the index of the channel needed.
> +- arm,smc-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.

What does each function do? I guess it signal that FW should look at a
particular mailbox, but the binding doesn't actually say.

If this is following the SMCCC, what is the function prototype?

What arguments are taken, and what return values are required under
which conditions?

Are they fast calls, or are they pre-emptible? When does the FW return?

If we're inventing a standard, why have multiple function IDs rather
than passing a channel ID in as a paarameter?

This needs a more rigorous definition.

> +
> +Optional properties:
> +--------------------
> +- method:		A string, either:
> +			"hvc": if the driver shall use an HVC call, or
> +			"smc": if the driver shall use an SMC call
> +			If omitted, defaults to an SMC call.

Make this property mandatory, don't guess. Please follow the wording
used by ther PSCI binding.

Given one can use HVC, the "arm,smc-func-ids" property is misleading,
and would be better as something like "func-ids".

Thanks,
Mark.

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

[Andre Przywara]

Hi,

On 07/07/17 15:35, Mark Rutland wrote:
> Hi Andre,
> 
> As a nit, please post bindings before drivers, as per
> Documentation/devicetree/bindings/submitting-patches.txt.

Sure.

> On Fri, Jun 30, 2017 at 10:56:02AM +0100, Andre Przywara wrote:
>> Add binding documentation for the generic ARM SMC mailbox.
>> This is not describing hardware, but a firmware interface.
> 
> Is this following some standard, or is this something that you have
> invented?

The idea was that we don't need a "full featured standard", but rather
just a contract between currently running firmware and the OS. Since I
consider the DT as being provided as part of the firmware, I don't see
how this would need to be "standardized" beyond that. This is just a
"single bit signalling channel" to implement a "poor man's mailbox", if
you like.
If firmware implements this, it adds the respective node, if not, there
is nothing in the DT.
Do you see any way this would clash with existing standards or would
need to standardized beyond this document? I would believe the SMCCC
covers the bits that would need to standardized already (as in to make
sure to not call any other service tied to a particular smc call).

> If the latter, why are we inventing a new standard? |How does
> this relate to SCPI and/or SCMI?

Both SCPI and SCMI build upon a shared memory region for transferring
arguments and a mailbox channel to signal that there is "work to do".
So this is not competing with those standards, but rather complementing
them and making them more widely usable (in cases where you don't have
an actual mailbox).
SCMI introduced a notion of multiple transport protocols for signalling,
but so far only specifies a mailbox (and does the respective proposed
binding).

Actually this driver is more of a drop-in replacement for an ARM MHU
mailbox (for instance to implement SCPI), so this driver and binding
actually promotes existing standards.

> What exactly is "the generic ARM SMC mailbox"?

That's a bridge between the requirements of having a mailbox (which the
SCPI *binding* requires, for instance) and some firmware parts being
implemented in an EL3 runtime on the APs (instead of running on some
separate management processors).
I am happy to change the name if that sounds presumptuous, the naming
comes from "generic" as in: does not require specific hardware. The
"ARM" in there is just to give context to "SMC", which, as every TLA, is
probably overloaded and confusing to the casual reader.

>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  .../devicetree/bindings/mailbox/arm-smc.txt        | 61 ++++++++++++++++++++++
>>  1 file changed, 61 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 0000000..90c5926
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt
>> @@ -0,0 +1,61 @@
>> +ARM SMC Mailbox Driver
> 
> Bindings do not document drivers. As Rob said, s/Driver/Interface/.

Right, badly copied from arm-mhu.txt ;-)

>> +======================
>> +
>> +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. 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.
> 
> ... where those values are what specifically, under what circumstances?

OK, I will look into how far this needs to be specified in this document
here or how much we can delegate to the actual mailbox users.

>> +One use case of this mailbox is the SCP 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.
>> +The communication follows the ARM SMC calling convention[1].
>> +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:
>> +====================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:		Shall be "arm,smc-mbox"
>> +- #mbox-cells		Shall be 1 - the index of the channel needed.
>> +- arm,smc-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.
> 
> What does each function do? I guess it signal that FW should look at a
> particular mailbox, but the binding doesn't actually say.

Calling the smc instruction with that function ID triggers the
associated mailbox (in the somewhat Linux/DT meaning of "mailbox").
Basically very much like the ARM MHU mailbox, for instance.

I believe the term mailbox is a bit fuzzy here. I think the least common
denominator just signals a single bit condition (like a doorbell), which
is the only thing that SCPI (for instance) requires. And the actual
mailbox content is then somewhere else, for instance in shared memory.
Which is beyond the scope of the mailbox itself.

> If this is following the SMCCC, what is the function prototype?

> What arguments are taken, and what return values are required under
> which conditions?

Good point, I can describe this one.

> Are they fast calls, or are they pre-emptible? When does the FW return?

Naturally I would expect fast calls, but I am not sure we need to
restrict it here. The SMCCC function ID encodes the type already. And I
couldn't find anything more detailed in the SMCCC to require me to ban
standard calls, so it would be up to the actual firmware implementation
to decide.

> If we're inventing a standard, why have multiple function IDs rather
> than passing a channel ID in as a paarameter?

What would be the advantage of doing so? I found it necessary to have
the SMCCC IDs in the DT anyway and also useful to support multiple
channels, so just having a list of SMCCC IDs sounded like a natural choice.
Do you reckon we need a combination of SMCCC ID and mailbox ID (as a
second parameter to the SMCCC call), to support cases where we only have
one SMCCC ID available, but need multiple mailboxes?

> This needs a more rigorous definition.
> 
>> +
>> +Optional properties:
>> +--------------------
>> +- method:		A string, either:
>> +			"hvc": if the driver shall use an HVC call, or
>> +			"smc": if the driver shall use an SMC call
>> +			If omitted, defaults to an SMC call.
> 
> Make this property mandatory, don't guess. Please follow the wording
> used by ther PSCI binding.

Sure.

> Given one can use HVC, the "arm,smc-func-ids" property is misleading,
> and would be better as something like "func-ids".

Right, will fix this.

Cheers,
Andre.

Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox

[André Przywara]

On 02/07/17 06:55, Jassi Brar wrote:

Hi Jassi,

thank you very much for having a look!

> On Fri, Jun 30, 2017 at 3:26 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> 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.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  drivers/mailbox/Kconfig           |   8 ++
>>  drivers/mailbox/Makefile          |   2 +
>>  drivers/mailbox/arm-smc-mailbox.c | 172 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 182 insertions(+)
>>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index c5731e5..5664b7f 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -170,4 +170,12 @@ config BCM_FLEXRM_MBOX
>>           Mailbox implementation of the Broadcom FlexRM ring manager,
>>           which provides access to various offload engines on Broadcom
>>           SoCs. Say Y here if you want to use the Broadcom FlexRM.
>> +
>> +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.
>> +
>>  endif
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index d54e412..8ec6869 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -35,3 +35,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX) += bcm-flexrm-mailbox.o
>>  obj-$(CONFIG_QCOM_APCS_IPC)    += qcom-apcs-ipc-mailbox.o
>>
>>  obj-$(CONFIG_TEGRA_HSP_MBOX)   += tegra-hsp.o
>> +
>> +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
>> diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
>> new file mode 100644
>> index 0000000..578aed2
>> --- /dev/null
>> +++ b/drivers/mailbox/arm-smc-mailbox.c
>> @@ -0,0 +1,172 @@
>> +/*
>> + *  Copyright (C) 2016,2017 ARM Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This device provides a mechanism for emulating a mailbox by using
>> + * smc calls, allowing a "mailbox" consumer to sit in firmware running
>> + * on the same core.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/arm-smccc.h>
>> +
> Please have relook at what headers are really needed.

Good point, indeed many of them are not needed.

>> +#define ARM_SMC_MBOX_SMC               (0 << 0)
>> +#define ARM_SMC_MBOX_HVC               (1 << 0)
>> +#define ARM_SMC_MBOX_METHOD_MASK       (1 << 0)
>> +
> Maybe have only
>    #define ARM_SMC_MBOX_HVC    BIT(0)
> 
>> +struct arm_smc_chan_data {
>> +       u32 function_id;
>> +       u32 flags;
>> +};
>> +
>> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
>> +{
>> +       struct arm_smc_chan_data *chan_data = link->con_priv;
>> +       u32 function_id = chan_data->function_id;
>> +       struct arm_smccc_res res;
>> +       u32 msg = *(u32 *)data;
>> +
>> +       if ((chan_data->flags & ARM_SMC_MBOX_METHOD_MASK) == ARM_SMC_MBOX_SMC)
>> +               arm_smccc_smc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
>> +       else
>> +               arm_smccc_hvc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
>> +
>        if (chan_data->flags & ARM_SMC_MBOX_HVC)
>                arm_smccc_hvc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
>        else
>                arm_smccc_smc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
> 
> is simpler.

Yeah, makes sense, I changed that.

>> +       mbox_chan_received_data(link, (void *)res.a0);
>> +
> Or you can update the 'data' with value from 'a0' ?

Mmh, I am a bit puzzled by this. Why would this be needed or useful? I
see that the SCPI firmware driver (as the user of the mailbox API) is
expecting the return value from a0 as returned above, translating the
firmware error codes into Linux' ones.
What am I missing here?

>> +       return 0;
>> +}
>> +
>> +static int arm_smc_startup(struct mbox_chan *link)
>> +{
>> +       return 0;
>> +}
>> +
>> +static void arm_smc_shutdown(struct mbox_chan *link)
>> +{
>> +}
>> +
> startup and shutdown can be omitted now.

Ah, thanks for the heads up, removed them.

>> +/* This mailbox is synchronous, so we are always done. */
>> +static bool arm_smc_last_tx_done(struct mbox_chan *link)
>> +{
>> +       return true;
>> +}
>> +
>> +static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
>> +       .send_data      = arm_smc_send_data,
>> +       .startup        = arm_smc_startup,
>> +       .shutdown       = arm_smc_shutdown,
>> +       .last_tx_done   = arm_smc_last_tx_done
>> +};
>> +
>> +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 = 0, i;
>> +
> No need to initialise 'ret'

Indeed!

Thanks for the comments!

Cheers,
Andre.

>> +       ret = of_property_count_elems_of_size(dev->of_node, "arm,smc-func-ids",
>> +                                             sizeof(u32));
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       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 = ret;
>> +       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,smc-func-ids", i,
>> +                                                &function_id);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               chan_data[i].function_id = function_id;
>> +               if (use_hvc)
>> +                       chan_data[i].flags |= ARM_SMC_MBOX_HVC;
>> +               mbox->chans[i].con_priv = &chan_data[i];
>> +       }
>> +
>> +       mbox->txdone_poll = true;
>> +       mbox->txdone_irq = false;
>> +       mbox->txpoll_period = 1;
>> +       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");
>> --
>> 2.9.0
>>

Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox

[Jassi Brar]

On Mon, Jul 24, 2017 at 4:50 AM, André Przywara <andre.przywara@arm.com> wrote:
> On 02/07/17 06:55, Jassi Brar wrote:
>
>>> +       mbox_chan_received_data(link, (void *)res.a0);
>>> +
>> Or you can update the 'data' with value from 'a0' ?
>
> Mmh, I am a bit puzzled by this. Why would this be needed or useful?
>
I meant instead of calling mbox_chan_received_data(), simply update
the value at 'data' with res.a0

Technically the firmware does not "send" us a message. It only updates
the structure we share with it. So maybe we could reflect that by
updating the data pointer the client driver asked to send.
Also it is optional for clients to provide the rx_callback(). By
calling mbox_chan_received_data() you mandate clients provide that
callback.

Nothing serious, just that looking closely, updating 'data' seems a
better option.

> I see that the SCPI firmware driver (as the user of the mailbox API) is
> expecting the return value from a0 as returned above, translating the
> firmware error codes into Linux' ones.
>
I am afraid, SCPI driver is not the golden example for client drivers
to follow. It is supposed to work only with MHU, and then, it is
likely to break if some other protocol is running parallel to it.

Thanks

Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox

[Sudeep Holla]

On 24/07/17 18:20, Jassi Brar wrote:
> On Mon, Jul 24, 2017 at 4:50 AM, André Przywara <andre.przywara@arm.com> wrote:
>> On 02/07/17 06:55, Jassi Brar wrote:
>>
>>>> +       mbox_chan_received_data(link, (void *)res.a0);
>>>> +
>>> Or you can update the 'data' with value from 'a0' ?
>>
>> Mmh, I am a bit puzzled by this. Why would this be needed or useful?
>>
> I meant instead of calling mbox_chan_received_data(), simply update
> the value at 'data' with res.a0
> 
> Technically the firmware does not "send" us a message. It only updates
> the structure we share with it. So maybe we could reflect that by
> updating the data pointer the client driver asked to send.
> Also it is optional for clients to provide the rx_callback(). By
> calling mbox_chan_received_data() you mandate clients provide that
> callback.
> 
> Nothing serious, just that looking closely, updating 'data' seems a
> better option.
> 
>> I see that the SCPI firmware driver (as the user of the mailbox API) is
>> expecting the return value from a0 as returned above, translating the
>> firmware error codes into Linux' ones.
>>
> I am afraid, SCPI driver is not the golden example for client drivers
> to follow. It is supposed to work only with MHU, and then, it is
> likely to break if some other protocol is running parallel to it.
> 

Not sure why do you say it works only with ARM MHU ? AmLogic uses it
with their mailbox driver. However they followed an interim version of
the SCPI spec which is termed "legacy" in the driver.
-- 
Regards,
Sudeep

Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox

[Jassi Brar]

On Mon, Jul 24, 2017 at 11:08 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 24/07/17 18:20, Jassi Brar wrote:

>>
>>> I see that the SCPI firmware driver (as the user of the mailbox API) is
>>> expecting the return value from a0 as returned above, translating the
>>> firmware error codes into Linux' ones.
>>>
>> I am afraid, SCPI driver is not the golden example for client drivers
>> to follow. It is supposed to work only with MHU, and then, it is
>> likely to break if some other protocol is running parallel to it.
>>
>
> Not sure why do you say it works only with ARM MHU ? AmLogic uses it
> with their mailbox driver. However they followed an interim version of
> the SCPI spec which is termed "legacy" in the driver.
>
Screw coding... Just tell me what stuff do you smoke? Must be really good!