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

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 <***@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

Andre Przywara

2017-06-30 09:56:01 UTC

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 <***@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 <***@arm.com>");
+MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
+MODULE_LICENSE("GPL v2");

--
2.9.0

Jassi Brar

2017-07-02 05:55:50 UTC

Post by 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.
---
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.

Post by Andre Przywara
+#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)

Post by Andre Przywara
+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.

Post by Andre Przywara
+ mbox_chan_received_data(link, (void *)res.a0);
+

Or you can update the 'data' with value from 'a0' ?

Post by Andre Przywara
+ 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.

Post by Andre Przywara
+/* 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'

Post by Andre Przywara
+ 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_DESCRIPTION("Generic ARM smc mailbox driver");
+MODULE_LICENSE("GPL v2");
--
2.9.0

André Przywara

2017-07-23 23:20:08 UTC

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

Hi Jassi,

thank you very much for having a look!

Please have relook at what headers are really needed.

Good point, indeed many of them are not needed.

Post by Andre Przywara
+#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)

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.

Post by Andre Przywara
+ 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?

Post by Andre Przywara
+ 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.

No need to initialise 'ret'

Indeed!

Thanks for the comments!

Cheers,
Andre.

Jassi Brar

2017-07-24 17:20:29 UTC

Post by AndrÃ© Przywara

Post by Andre Przywara
+ 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.

Post by AndrÃ© Przywara
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

Sudeep Holla

2017-07-24 17:38:23 UTC

Post by AndrÃ© Przywara

Post by Andre Przywara
+ 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.

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

Jassi Brar

2017-07-24 17:52:41 UTC

Post by Sudeep Holla

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!

Andre Przywara

2017-06-30 09:56:04 UTC

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 <***@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: ***@0 {
+ compatible = "arm,smc-mbox";
+ #mbox-cells = <1>;
+ arm,smc-func-ids = <0x82000001>;
+ };
+
+ sram: ***@10000{
+ compatible = "mmio-sram";
+ reg = <0x10000 0x8000>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0x10000 0x8000>;
+
+ cpu_scp_mem: scp-***@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

Andre Przywara

2017-06-30 09:56:07 UTC

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 <***@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

Andre Przywara

2017-06-30 09:56:06 UTC

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 <***@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: ***@1c20800 {

--
2.9.0

Andre Przywara

2017-06-30 09:56:05 UTC

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 <***@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: ***@1 {
@@ -68,6 +69,7 @@
device_type = "cpu";
reg = <1>;
enable-method = "psci";
+ clocks = <&scpi_dvfs 0>;
};

cpu2: ***@2 {
@@ -75,6 +77,7 @@
device_type = "cpu";
reg = <2>;
enable-method = "psci";
+ clocks = <&scpi_dvfs 0>;
};

cpu3: ***@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

Andre Przywara

2017-06-30 09:56:08 UTC

This post might be inappropriate. Click to display it.

Maxime Ripard

2017-06-30 12:25:29 UTC

Hi,

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

Andre Przywara

2017-06-30 12:56:04 UTC

Hi,

thanks for having a look!

Post by Maxime Ripard
Hi,

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.

Maxime Ripard

2017-07-05 06:55:59 UTC

Post by Andre Przywara
Hi,
thanks for having a look!

Post by Maxime Ripard
Hi,

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.

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

Rob Herring

2017-07-07 13:53:25 UTC

Post by Andre Przywara
Add binding documentation for the generic ARM SMC mailbox.
This is not describing hardware, but a firmware interface.
---
.../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/

Post by Andre Przywara
+======================
+
+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.
+
+====================

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

Post by Andre Przywara
+
+--------------------
+- 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.
+
+--------------------
+ "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.
+
+--------
+
+ mailbox: smc_mbox {

swap these. mailbox for the node name.

Post by Andre Przywara
+ #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

Mark Rutland

2017-07-07 14:35:46 UTC

Hi Andre,

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

Post by Andre Przywara
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"?

Post by Andre Przywara
---
.../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/.

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

Post by Andre Przywara
+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.
+
+====================
+
+--------------------
+- 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.

Post by Andre Przywara
+
+--------------------
+ "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.

Andre Przywara

2017-07-07 16:06:39 UTC

Hi,

Post by Mark Rutland
Hi Andre,
As a nit, please post bindings before drivers, as per
Documentation/devicetree/bindings/submitting-patches.txt.

Sure.

Post by Andre Przywara
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).

Post by Mark Rutland
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.

Post by Mark Rutland
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.

Bindings do not document drivers. As Rob said, s/Driver/Interface/.

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

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