* [PATCH 0/3] remoteproc: introduce Arm remoteproc support
@ 2024-03-01 16:42 abdellatif.elkhlifi
2024-03-01 16:42 ` [PATCH 1/3] remoteproc: Add Arm remoteproc driver abdellatif.elkhlifi
` (2 more replies)
0 siblings, 3 replies; 59+ messages in thread
From: abdellatif.elkhlifi @ 2024-03-01 16:42 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring
Cc: Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, Abdellatif El Khlifi,
linux-arm-kernel, devicetree, linux-kernel, linux-remoteproc
From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Some Arm heterogeneous System-On-Chips feature remote processors that can
be controlled with a reset control register and a reset status register to
start or stop the processor.
This patchset adds support for these processors by providing the
following:
1) A remoteproc driver that retrieves the reset registers addresses from
the DT, register a new rproc device with the remoteproc subsystem and
provides the start and stop operations for switching on or off the remote
processor.
The start and stop operations are provided as a data config selected on DT node match.
Currently we are providing support for Corstone-1000 External System (Cortex-M3) [1]
as a remote processor. The driver can be extended to support other remote processors
by adding a data config and custom implementation of the start and stop operations.
2) DT bindings
3) Support control of multiple remote processors at the same time
[1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-1000
Cheers,
Abdellatif
Abdellatif El Khlifi (3):
remoteproc: Add Arm remoteproc driver
arm64: dts: Add corstone1000 external system device node
dt-bindings: remoteproc: Add Arm remoteproc
.../bindings/remoteproc/arm,rproc.yaml | 69 +++
MAINTAINERS | 7 +
arch/arm64/boot/dts/arm/corstone1000.dtsi | 10 +-
drivers/remoteproc/Kconfig | 18 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/arm_rproc.c | 395 ++++++++++++++++++
6 files changed, 499 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
create mode 100644 drivers/remoteproc/arm_rproc.c
base-commit: 8b46dc5cfa5ffea279aed0fc05dc4b1c39a51517
--
2.25.1
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-01 16:42 [PATCH 0/3] remoteproc: introduce Arm remoteproc support abdellatif.elkhlifi
@ 2024-03-01 16:42 ` abdellatif.elkhlifi
2024-03-04 18:30 ` Rob Herring
2024-03-04 18:42 ` Mathieu Poirier
2024-03-01 16:42 ` [PATCH 2/3] arm64: dts: Add corstone1000 external system device node abdellatif.elkhlifi
2024-03-01 16:42 ` [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc abdellatif.elkhlifi
2 siblings, 2 replies; 59+ messages in thread
From: abdellatif.elkhlifi @ 2024-03-01 16:42 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring
Cc: Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, Abdellatif El Khlifi,
linux-arm-kernel, devicetree, linux-kernel, linux-remoteproc
From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
introduce remoteproc support for Arm remote processors
The supported remote processors are those that come with a reset
control register and a reset status register. The driver allows to
switch on or off the remote processor.
The current use case is Corstone-1000 External System (Cortex-M3).
The driver can be extended to support other remote processors
controlled with a reset control and a reset status registers.
The driver also supports control of multiple remote processors at the
same time.
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
MAINTAINERS | 6 +
drivers/remoteproc/Kconfig | 18 ++
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/arm_rproc.c | 395 +++++++++++++++++++++++++++++++++
4 files changed, 420 insertions(+)
create mode 100644 drivers/remoteproc/arm_rproc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d1052fa6a69..54d6a40feea5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1764,6 +1764,12 @@ S: Maintained
F: Documentation/devicetree/bindings/interrupt-controller/arm,vic.yaml
F: drivers/irqchip/irq-vic.c
+ARM REMOTEPROC DRIVER
+M: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
+L: linux-remoteproc@vger.kernel.org
+S: Maintained
+F: drivers/remoteproc/arm_rproc.c
+
ARM SMC WATCHDOG DRIVER
M: Julius Werner <jwerner@chromium.org>
R: Evan Benn <evanbenn@chromium.org>
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48845dc8fa85..57fbac454a5d 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -365,6 +365,24 @@ config XLNX_R5_REMOTEPROC
It's safe to say N if not interested in using RPU r5f cores.
+config ARM_REMOTEPROC
+ tristate "Arm remoteproc support"
+ depends on HAS_IOMEM && ARM64
+ default n
+ help
+ Say y here to support Arm remote processors via the remote
+ processor framework.
+
+ The supported processors are those that come with a reset control register
+ and a reset status register. The design can be extended to support different
+ processors meeting these requirements.
+ The driver also supports control of multiple remote cores at the same time.
+
+ Supported remote cores:
+ Corstone-1000 External System (Cortex-M3)
+
+ It's safe to say N here.
+
endif # REMOTEPROC
endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 91314a9b43ce..73126310835b 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -39,3 +39,4 @@ obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
obj-$(CONFIG_XLNX_R5_REMOTEPROC) += xlnx_r5_remoteproc.o
+obj-$(CONFIG_ARM_REMOTEPROC) += arm_rproc.o
diff --git a/drivers/remoteproc/arm_rproc.c b/drivers/remoteproc/arm_rproc.c
new file mode 100644
index 000000000000..6afa78ae7ad3
--- /dev/null
+++ b/drivers/remoteproc/arm_rproc.c
@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Arm Limited and/or its affiliates <open-source-office@arm.com>
+ *
+ * Authors:
+ * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+/**
+ * struct arm_rproc_reset_cfg - remote processor reset configuration
+ * @ctrl_reg: address of the control register
+ * @state_reg: address of the reset status register
+ */
+struct arm_rproc_reset_cfg {
+ void __iomem *ctrl_reg;
+ void __iomem *state_reg;
+};
+
+struct arm_rproc;
+
+/**
+ * struct arm_rproc_dcfg - Arm remote processor configuration
+ * @stop: stop callback function
+ * @start: start callback function
+ */
+struct arm_rproc_dcfg {
+ int (*stop)(struct rproc *rproc);
+ int (*start)(struct rproc *rproc);
+};
+
+/**
+ * struct arm_rproc - Arm remote processor instance
+ * @rproc: rproc handler
+ * @core_dcfg: device configuration pointer
+ * @reset_cfg: reset configuration registers
+ */
+struct arm_rproc {
+ struct rproc *rproc;
+ const struct arm_rproc_dcfg *core_dcfg;
+ struct arm_rproc_reset_cfg reset_cfg;
+};
+
+/* Definitions for Arm Corstone-1000 External System */
+
+#define EXTSYS_RST_CTRL_CPUWAIT BIT(0)
+#define EXTSYS_RST_CTRL_RST_REQ BIT(1)
+
+#define EXTSYS_RST_ACK_MASK GENMASK(2, 1)
+#define EXTSYS_RST_ST_RST_ACK(x) \
+ ((u8)(FIELD_GET(EXTSYS_RST_ACK_MASK, (x))))
+
+#define EXTSYS_RST_ACK_NO_RESET_REQ (0x0)
+#define EXTSYS_RST_ACK_NOT_COMPLETE (0x1)
+#define EXTSYS_RST_ACK_COMPLETE (0x2)
+#define EXTSYS_RST_ACK_RESERVED (0x3)
+
+#define EXTSYS_RST_ACK_POLL_TRIES (3)
+#define EXTSYS_RST_ACK_POLL_TIMEOUT (1000)
+
+/**
+ * arm_rproc_start_cs1000_extsys() - custom start function
+ * @rproc: pointer to the remote processor object
+ *
+ * Start function for Corstone-1000 External System.
+ * Allow the External System core start execute instructions.
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int arm_rproc_start_cs1000_extsys(struct rproc *rproc)
+{
+ struct arm_rproc *priv = rproc->priv;
+ u32 ctrl_reg;
+
+ /* CPUWAIT signal of the External System is de-asserted */
+ ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
+ ctrl_reg &= ~EXTSYS_RST_CTRL_CPUWAIT;
+ writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
+
+ return 0;
+}
+
+/**
+ * arm_rproc_cs1000_extsys_poll_rst_ack() - poll RST_ACK bits
+ * @rproc: pointer to the remote processor object
+ * @exp_ack: expected bits value
+ * @rst_ack: bits value read
+ *
+ * Tries to read RST_ACK bits until the timeout expires.
+ * EXTSYS_RST_ACK_POLL_TRIES tries are made,
+ * every EXTSYS_RST_ACK_POLL_TIMEOUT milliseconds.
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int arm_rproc_cs1000_extsys_poll_rst_ack(struct rproc *rproc,
+ u8 exp_ack, u8 *rst_ack)
+{
+ struct arm_rproc *priv = rproc->priv;
+ struct device *dev = rproc->dev.parent;
+ u32 state_reg;
+ int tries = EXTSYS_RST_ACK_POLL_TRIES;
+ unsigned long timeout;
+
+ do {
+ state_reg = readl(priv->reset_cfg.state_reg);
+ *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
+
+ if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
+ dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
+ *rst_ack);
+ return -EINVAL;
+ }
+
+ /* expected ACK value read */
+ if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
+ return 0;
+
+ timeout = msleep_interruptible(EXTSYS_RST_ACK_POLL_TIMEOUT);
+
+ if (timeout) {
+ dev_err(dev, "polling RST_ACK aborted\n");
+ return -ECONNABORTED;
+ }
+ } while (--tries);
+
+ dev_err(dev, "polling RST_ACK timed out\n");
+
+ return -ETIMEDOUT;
+}
+
+/**
+ * arm_rproc_stop_cs1000_extsys() - custom stop function
+ * @rproc: pointer to the remote processor object
+ *
+ * Reset all logic within the External System, the core will be in a halt state.
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int arm_rproc_stop_cs1000_extsys(struct rproc *rproc)
+{
+ struct arm_rproc *priv = rproc->priv;
+ struct device *dev = rproc->dev.parent;
+ u32 ctrl_reg;
+ u8 rst_ack, req_status;
+ int ret;
+
+ ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
+ ctrl_reg |= EXTSYS_RST_CTRL_RST_REQ;
+ writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
+
+ ret = arm_rproc_cs1000_extsys_poll_rst_ack(rproc,
+ EXTSYS_RST_ACK_COMPLETE |
+ EXTSYS_RST_ACK_NOT_COMPLETE,
+ &rst_ack);
+ if (ret)
+ return ret;
+
+ req_status = rst_ack;
+
+ ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
+ ctrl_reg &= ~EXTSYS_RST_CTRL_RST_REQ;
+ writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
+
+ ret = arm_rproc_cs1000_extsys_poll_rst_ack(rproc, 0, &rst_ack);
+ if (ret)
+ return ret;
+
+ if (req_status == EXTSYS_RST_ACK_COMPLETE) {
+ dev_dbg(dev, "the requested reset has been accepted\n");
+ return 0;
+ }
+
+ dev_err(dev, "the requested reset has been denied\n");
+ return -EACCES;
+}
+
+static const struct arm_rproc_dcfg arm_rproc_cfg_corstone1000_extsys = {
+ .stop = arm_rproc_stop_cs1000_extsys,
+ .start = arm_rproc_start_cs1000_extsys,
+};
+
+/**
+ * arm_rproc_stop() - Stop function for rproc_ops
+ * @rproc: pointer to the remote processor object
+ *
+ * Calls the stop() callback of the remote core
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int arm_rproc_stop(struct rproc *rproc)
+{
+ struct arm_rproc *priv = rproc->priv;
+
+ return priv->core_dcfg->stop(rproc);
+}
+
+/**
+ * arm_rproc_start() - Start function for rproc_ops
+ * @rproc: pointer to the remote processor object
+ *
+ * Calls the start() callback of the remote core
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int arm_rproc_start(struct rproc *rproc)
+{
+ struct arm_rproc *priv = rproc->priv;
+
+ return priv->core_dcfg->start(rproc);
+}
+
+/**
+ * arm_rproc_parse_fw() - Parse firmware function for rproc_ops
+ * @rproc: pointer to the remote processor object
+ * @fw: pointer to the firmware
+ *
+ * Does nothing currently.
+ *
+ * Return:
+ *
+ * 0 for success.
+ */
+static int arm_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+ return 0;
+}
+
+/**
+ * arm_rproc_load() - Load firmware to memory function for rproc_ops
+ * @rproc: pointer to the remote processor object
+ * @fw: pointer to the firmware
+ *
+ * Does nothing currently.
+ *
+ * Return:
+ *
+ * 0 for success.
+ */
+static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
+{
+ return 0;
+}
+
+static const struct rproc_ops arm_rproc_ops = {
+ .start = arm_rproc_start,
+ .stop = arm_rproc_stop,
+ .load = arm_rproc_load,
+ .parse_fw = arm_rproc_parse_fw,
+};
+
+/**
+ * arm_rproc_probe() - the platform device probe
+ * @pdev: the platform device
+ *
+ * Read from the device tree the properties needed to setup
+ * the reset and comms for the remote processor.
+ * Also, allocate a rproc device and register it with the remoteproc subsystem.
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int arm_rproc_probe(struct platform_device *pdev)
+{
+ const struct arm_rproc_dcfg *core_dcfg;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct arm_rproc *priv;
+ struct rproc *rproc;
+ const char *fw_name;
+ int ret;
+ struct resource *res;
+
+ core_dcfg = of_device_get_match_data(dev);
+ if (!core_dcfg)
+ return -ENODEV;
+
+ ret = rproc_of_parse_firmware(dev, 0, &fw_name);
+ if (ret) {
+ dev_err(dev,
+ "can't parse firmware-name from device tree (%pe)\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+
+ dev_dbg(dev, "firmware-name: %s\n", fw_name);
+
+ rproc = rproc_alloc(dev, np->name, &arm_rproc_ops, fw_name,
+ sizeof(*priv));
+ if (!rproc)
+ return -ENOMEM;
+
+ priv = rproc->priv;
+ priv->rproc = rproc;
+ priv->core_dcfg = core_dcfg;
+
+ res = platform_get_resource_byname(pdev,
+ IORESOURCE_MEM, "reset-control");
+ priv->reset_cfg.ctrl_reg = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(priv->reset_cfg.ctrl_reg)) {
+ ret = PTR_ERR(priv->reset_cfg.ctrl_reg);
+ dev_err(dev,
+ "can't map the reset-control register (%pe)\n",
+ ERR_PTR((unsigned long)priv->reset_cfg.ctrl_reg));
+ goto err_free_rproc;
+ } else {
+ dev_dbg(dev, "reset-control: %p\n", priv->reset_cfg.ctrl_reg);
+ }
+
+ res = platform_get_resource_byname(pdev,
+ IORESOURCE_MEM, "reset-status");
+ priv->reset_cfg.state_reg = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(priv->reset_cfg.state_reg)) {
+ ret = PTR_ERR(priv->reset_cfg.state_reg);
+ dev_err(dev,
+ "can't map the reset-status register (%pe)\n",
+ ERR_PTR((unsigned long)priv->reset_cfg.state_reg));
+ goto err_free_rproc;
+ } else {
+ dev_dbg(dev, "reset-status: %p\n",
+ priv->reset_cfg.state_reg);
+ }
+
+ platform_set_drvdata(pdev, rproc);
+
+ ret = rproc_add(rproc);
+ if (ret) {
+ dev_err(dev, "can't add remote processor (%pe)\n",
+ ERR_PTR(ret));
+ goto err_free_rproc;
+ } else {
+ dev_dbg(dev, "remote processor added\n");
+ }
+
+ return 0;
+
+err_free_rproc:
+ rproc_free(rproc);
+
+ return ret;
+}
+
+/**
+ * arm_rproc_remove() - the platform device remove
+ * @pdev: the platform device
+ *
+ * Delete and free the resources used.
+ */
+static void arm_rproc_remove(struct platform_device *pdev)
+{
+ struct rproc *rproc = platform_get_drvdata(pdev);
+
+ rproc_del(rproc);
+ rproc_free(rproc);
+}
+
+static const struct of_device_id arm_rproc_of_match[] = {
+ { .compatible = "arm,corstone1000-extsys", .data = &arm_rproc_cfg_corstone1000_extsys },
+ {},
+};
+MODULE_DEVICE_TABLE(of, arm_rproc_of_match);
+
+static struct platform_driver arm_rproc_driver = {
+ .probe = arm_rproc_probe,
+ .remove_new = arm_rproc_remove,
+ .driver = {
+ .name = "arm-rproc",
+ .of_match_table = arm_rproc_of_match,
+ },
+};
+module_platform_driver(arm_rproc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Arm Remote Processor Control Driver");
+MODULE_AUTHOR("Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>");
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 2/3] arm64: dts: Add corstone1000 external system device node
2024-03-01 16:42 [PATCH 0/3] remoteproc: introduce Arm remoteproc support abdellatif.elkhlifi
2024-03-01 16:42 ` [PATCH 1/3] remoteproc: Add Arm remoteproc driver abdellatif.elkhlifi
@ 2024-03-01 16:42 ` abdellatif.elkhlifi
2024-03-01 19:27 ` Krzysztof Kozlowski
2024-03-08 12:21 ` Sudeep Holla
2024-03-01 16:42 ` [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc abdellatif.elkhlifi
2 siblings, 2 replies; 59+ messages in thread
From: abdellatif.elkhlifi @ 2024-03-01 16:42 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring
Cc: Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, Abdellatif El Khlifi,
linux-arm-kernel, devicetree, linux-kernel, linux-remoteproc
From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
add device tree node for the external system core in Corstone-1000
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
arch/arm64/boot/dts/arm/corstone1000.dtsi | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/arm/corstone1000.dtsi b/arch/arm64/boot/dts/arm/corstone1000.dtsi
index 6ad7829f9e28..67df642363e9 100644
--- a/arch/arm64/boot/dts/arm/corstone1000.dtsi
+++ b/arch/arm64/boot/dts/arm/corstone1000.dtsi
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0 OR MIT
/*
- * Copyright (c) 2022, Arm Limited. All rights reserved.
+ * Copyright 2022, 2024, Arm Limited and/or its affiliates <open-source-office@arm.com>
* Copyright (c) 2022, Linaro Limited. All rights reserved.
*
*/
@@ -157,5 +157,13 @@ mhu_seh1: mailbox@1b830000 {
secure-status = "okay"; /* secure-world-only */
status = "disabled";
};
+
+ extsys0: remoteproc@1a010310 {
+ compatible = "arm,corstone1000-extsys";
+ reg = <0x1a010310 0x4>,
+ <0x1a010314 0X4>;
+ reg-names = "reset-control", "reset-status";
+ firmware-name = "es_flashfw.elf";
+ };
};
};
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-01 16:42 [PATCH 0/3] remoteproc: introduce Arm remoteproc support abdellatif.elkhlifi
2024-03-01 16:42 ` [PATCH 1/3] remoteproc: Add Arm remoteproc driver abdellatif.elkhlifi
2024-03-01 16:42 ` [PATCH 2/3] arm64: dts: Add corstone1000 external system device node abdellatif.elkhlifi
@ 2024-03-01 16:42 ` abdellatif.elkhlifi
2024-03-01 19:30 ` Krzysztof Kozlowski
2024-03-13 19:59 ` Robin Murphy
2 siblings, 2 replies; 59+ messages in thread
From: abdellatif.elkhlifi @ 2024-03-01 16:42 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring
Cc: Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, Abdellatif El Khlifi,
linux-arm-kernel, devicetree, linux-kernel, linux-remoteproc
From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
introduce the bindings for Arm remoteproc support.
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
.../bindings/remoteproc/arm,rproc.yaml | 69 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 70 insertions(+)
create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
new file mode 100644
index 000000000000..322197158059
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/arm,rproc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm Remoteproc Devices
+
+maintainers:
+ - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
+
+description: |
+ Some Arm heterogeneous System-On-Chips feature remote processors that can
+ be controlled with a reset control register and a reset status register to
+ start or stop the processor.
+
+ This document defines the bindings for these remote processors.
+
+properties:
+ compatible:
+ enum:
+ - arm,corstone1000-extsys
+
+ reg:
+ minItems: 2
+ maxItems: 2
+ description: |
+ Address and size in bytes of the reset control register
+ and the reset status register.
+ Expects the registers to be in the order as above.
+ Should contain an entry for each value in 'reg-names'.
+
+ reg-names:
+ description: |
+ Required names for each of the reset registers defined in
+ the 'reg' property. Expects the names from the following
+ list, in the specified order, each representing the corresponding
+ reset register.
+ items:
+ - const: reset-control
+ - const: reset-status
+
+ firmware-name:
+ description: |
+ Default name of the firmware to load to the remote processor.
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - firmware-name
+
+additionalProperties: false
+
+examples:
+ - |
+ extsys0: remoteproc@1a010310 {
+ compatible = "arm,corstone1000-extsys";
+ reg = <0x1a010310 0x4>, <0x1a010314 0x4>;
+ reg-names = "reset-control", "reset-status";
+ firmware-name = "es0_flashfw.elf";
+ };
+
+ extsys1: remoteproc@1a010318 {
+ compatible = "arm,corstone1000-extsys";
+ reg = <0x1a010318 0x4>, <0x1a01031c 0x4>;
+ reg-names = "reset-control", "reset-status";
+ firmware-name = "es1_flashfw.elf";
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 54d6a40feea5..eddaa3841a65 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1768,6 +1768,7 @@ ARM REMOTEPROC DRIVER
M: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
L: linux-remoteproc@vger.kernel.org
S: Maintained
+F: Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
F: drivers/remoteproc/arm_rproc.c
ARM SMC WATCHDOG DRIVER
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH 2/3] arm64: dts: Add corstone1000 external system device node
2024-03-01 16:42 ` [PATCH 2/3] arm64: dts: Add corstone1000 external system device node abdellatif.elkhlifi
@ 2024-03-01 19:27 ` Krzysztof Kozlowski
2024-03-08 12:21 ` Sudeep Holla
1 sibling, 0 replies; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-01 19:27 UTC (permalink / raw)
To: abdellatif.elkhlifi, Bjorn Andersson, Mathieu Poirier,
Rob Herring
Cc: Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
On 01/03/2024 17:42, abdellatif.elkhlifi@arm.com wrote:
> From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>
> add device tree node for the external system core in Corstone-1000
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> arch/arm64/boot/dts/arm/corstone1000.dtsi | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/arm/corstone1000.dtsi b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> index 6ad7829f9e28..67df642363e9 100644
> --- a/arch/arm64/boot/dts/arm/corstone1000.dtsi
> +++ b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0 OR MIT
> /*
> - * Copyright (c) 2022, Arm Limited. All rights reserved.
> + * Copyright 2022, 2024, Arm Limited and/or its affiliates <open-source-office@arm.com>
> * Copyright (c) 2022, Linaro Limited. All rights reserved.
> *
> */
> @@ -157,5 +157,13 @@ mhu_seh1: mailbox@1b830000 {
> secure-status = "okay"; /* secure-world-only */
> status = "disabled";
> };
> +
> + extsys0: remoteproc@1a010310 {
Looks not really ordered.
> + compatible = "arm,corstone1000-extsys";
> + reg = <0x1a010310 0x4>,
> + <0x1a010314 0X4>;
And this needs alignment.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-01 16:42 ` [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc abdellatif.elkhlifi
@ 2024-03-01 19:30 ` Krzysztof Kozlowski
2024-03-08 12:29 ` Sudeep Holla
2024-03-13 19:59 ` Robin Murphy
1 sibling, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-01 19:30 UTC (permalink / raw)
To: abdellatif.elkhlifi, Bjorn Andersson, Mathieu Poirier,
Rob Herring
Cc: Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
On 01/03/2024 17:42, abdellatif.elkhlifi@arm.com wrote:
> From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>
> introduce the bindings for Arm remoteproc support.
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> .../bindings/remoteproc/arm,rproc.yaml | 69 +++++++++++++++++++
> MAINTAINERS | 1 +
Fix order of patches - bindings are always before the user (see
submitting bindings doc).
> 2 files changed, 70 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> new file mode 100644
> index 000000000000..322197158059
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/arm,rproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Arm Remoteproc Devices
That's quite generic... does it applied to all ARM designs?
> +
> +maintainers:
> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> +
> +description: |
> + Some Arm heterogeneous System-On-Chips feature remote processors that can
> + be controlled with a reset control register and a reset status register to
> + start or stop the processor.
> +
> + This document defines the bindings for these remote processors.
Drop last sentence.
> +
> +properties:
> + compatible:
> + enum:
> + - arm,corstone1000-extsys
> +
> + reg:
> + minItems: 2
> + maxItems: 2
> + description: |
> + Address and size in bytes of the reset control register
> + and the reset status register.
> + Expects the registers to be in the order as above.
> + Should contain an entry for each value in 'reg-names'.
Entirely redundant sentences... instead this all just list items with
description.
> +
> + reg-names:Do not need '|' unless you need to preserve formatting.
> + description: |
> + Required names for each of the reset registers defined in
> + the 'reg' property. Expects the names from the following
> + list, in the specified order, each representing the corresponding
> + reset register.
Really, drop.
> + items:
> + - const: reset-control
> + - const: reset-status
> +
> + firmware-name:
> + description: |
Do not need '|' unless you need to preserve formatting.
> + Default name of the firmware to load to the remote processor.
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> + - firmware-name
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + extsys0: remoteproc@1a010310 {
Drop label, not used.
> + compatible = "arm,corstone1000-extsys";
Use 4 spaces for example indentation.
> + reg = <0x1a010310 0x4>, <0x1a010314 0x4>;
> + reg-names = "reset-control", "reset-status";
> + firmware-name = "es0_flashfw.elf";
> + };
> +
> + extsys1: remoteproc@1a010318 {
> + compatible = "arm,corstone1000-extsys";
These are the same examples, so keep only one.
> + reg = <0x1a010318 0x4>, <0x1a01031c 0x4>;
> + reg-names = "reset-control", "reset-status";
> + firmware-name = "es1_flashfw.elf";
> + };
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-01 16:42 ` [PATCH 1/3] remoteproc: Add Arm remoteproc driver abdellatif.elkhlifi
@ 2024-03-04 18:30 ` Rob Herring
2024-03-04 18:42 ` Mathieu Poirier
1 sibling, 0 replies; 59+ messages in thread
From: Rob Herring @ 2024-03-04 18:30 UTC (permalink / raw)
To: abdellatif.elkhlifi
Cc: Bjorn Andersson, Mathieu Poirier, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
On Fri, Mar 01, 2024 at 04:42:25PM +0000, abdellatif.elkhlifi@arm.com wrote:
> From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>
> introduce remoteproc support for Arm remote processors
>
> The supported remote processors are those that come with a reset
> control register and a reset status register. The driver allows to
> switch on or off the remote processor.
>
> The current use case is Corstone-1000 External System (Cortex-M3).
>
> The driver can be extended to support other remote processors
> controlled with a reset control and a reset status registers.
>
> The driver also supports control of multiple remote processors at the
> same time.
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> MAINTAINERS | 6 +
> drivers/remoteproc/Kconfig | 18 ++
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/arm_rproc.c | 395 +++++++++++++++++++++++++++++++++
> 4 files changed, 420 insertions(+)
> create mode 100644 drivers/remoteproc/arm_rproc.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d1052fa6a69..54d6a40feea5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1764,6 +1764,12 @@ S: Maintained
> F: Documentation/devicetree/bindings/interrupt-controller/arm,vic.yaml
> F: drivers/irqchip/irq-vic.c
>
> +ARM REMOTEPROC DRIVER
> +M: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> +L: linux-remoteproc@vger.kernel.org
> +S: Maintained
> +F: drivers/remoteproc/arm_rproc.c
> +
> ARM SMC WATCHDOG DRIVER
> M: Julius Werner <jwerner@chromium.org>
> R: Evan Benn <evanbenn@chromium.org>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 48845dc8fa85..57fbac454a5d 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -365,6 +365,24 @@ config XLNX_R5_REMOTEPROC
>
> It's safe to say N if not interested in using RPU r5f cores.
>
> +config ARM_REMOTEPROC
> + tristate "Arm remoteproc support"
Too generic of a name. It should say Corstone or Corstone-1000. Here and
everywhere you use just 'Arm'.
> + depends on HAS_IOMEM && ARM64
depends on ARM64 || (HAS_IOMEM && COMPILE_TEST)
That gets us wider build coverage. You should check at least x86
allmodconfig passes.
> + default n
The default is already n, so drop.
> + help
> + Say y here to support Arm remote processors via the remote
> + processor framework.
> +
> + The supported processors are those that come with a reset control register
> + and a reset status register. The design can be extended to support different
> + processors meeting these requirements.
> + The driver also supports control of multiple remote cores at the same time.
> +
> + Supported remote cores:
> + Corstone-1000 External System (Cortex-M3)
> +
> + It's safe to say N here.
> +
> endif # REMOTEPROC
>
> endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 91314a9b43ce..73126310835b 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -39,3 +39,4 @@ obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
> obj-$(CONFIG_XLNX_R5_REMOTEPROC) += xlnx_r5_remoteproc.o
> +obj-$(CONFIG_ARM_REMOTEPROC) += arm_rproc.o
> diff --git a/drivers/remoteproc/arm_rproc.c b/drivers/remoteproc/arm_rproc.c
> new file mode 100644
> index 000000000000..6afa78ae7ad3
> --- /dev/null
> +++ b/drivers/remoteproc/arm_rproc.c
> @@ -0,0 +1,395 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Arm Limited and/or its affiliates <open-source-office@arm.com>
We don't normally put OSO email in here.
> + *
> + * Authors:
> + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
That's recorded in the commit message and by git, so no need to put it
in the file.
> + */
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-01 16:42 ` [PATCH 1/3] remoteproc: Add Arm remoteproc driver abdellatif.elkhlifi
2024-03-04 18:30 ` Rob Herring
@ 2024-03-04 18:42 ` Mathieu Poirier
2024-03-07 19:40 ` Abdellatif El Khlifi
1 sibling, 1 reply; 59+ messages in thread
From: Mathieu Poirier @ 2024-03-04 18:42 UTC (permalink / raw)
To: abdellatif.elkhlifi
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
Good day Abdellatif,
On Fri, Mar 01, 2024 at 04:42:25PM +0000, abdellatif.elkhlifi@arm.com wrote:
> From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>
> introduce remoteproc support for Arm remote processors
>
> The supported remote processors are those that come with a reset
> control register and a reset status register. The driver allows to
> switch on or off the remote processor.
>
> The current use case is Corstone-1000 External System (Cortex-M3).
>
> The driver can be extended to support other remote processors
> controlled with a reset control and a reset status registers.
>
> The driver also supports control of multiple remote processors at the
> same time.
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> MAINTAINERS | 6 +
> drivers/remoteproc/Kconfig | 18 ++
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/arm_rproc.c | 395 +++++++++++++++++++++++++++++++++
> 4 files changed, 420 insertions(+)
> create mode 100644 drivers/remoteproc/arm_rproc.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d1052fa6a69..54d6a40feea5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1764,6 +1764,12 @@ S: Maintained
> F: Documentation/devicetree/bindings/interrupt-controller/arm,vic.yaml
> F: drivers/irqchip/irq-vic.c
>
> +ARM REMOTEPROC DRIVER
> +M: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> +L: linux-remoteproc@vger.kernel.org
> +S: Maintained
> +F: drivers/remoteproc/arm_rproc.c
> +
Humm... I'm not sure this is needed for now. You'll be CC'ed in future postings
anyway if someone changes this drivers.
> ARM SMC WATCHDOG DRIVER
> M: Julius Werner <jwerner@chromium.org>
> R: Evan Benn <evanbenn@chromium.org>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 48845dc8fa85..57fbac454a5d 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -365,6 +365,24 @@ config XLNX_R5_REMOTEPROC
>
> It's safe to say N if not interested in using RPU r5f cores.
>
> +config ARM_REMOTEPROC
> + tristate "Arm remoteproc support"
> + depends on HAS_IOMEM && ARM64
> + default n
> + help
> + Say y here to support Arm remote processors via the remote
> + processor framework.
> +
> + The supported processors are those that come with a reset control register
> + and a reset status register. The design can be extended to support different
> + processors meeting these requirements.
> + The driver also supports control of multiple remote cores at the same time.
> +
> + Supported remote cores:
> + Corstone-1000 External System (Cortex-M3)
> +
Please remove. The descrition in the Kconfig file should be related to a family
of device and the specific model number found in the driver.
> + It's safe to say N here.
> +
> endif # REMOTEPROC
>
> endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 91314a9b43ce..73126310835b 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -39,3 +39,4 @@ obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
> obj-$(CONFIG_XLNX_R5_REMOTEPROC) += xlnx_r5_remoteproc.o
> +obj-$(CONFIG_ARM_REMOTEPROC) += arm_rproc.o
> diff --git a/drivers/remoteproc/arm_rproc.c b/drivers/remoteproc/arm_rproc.c
> new file mode 100644
> index 000000000000..6afa78ae7ad3
> --- /dev/null
> +++ b/drivers/remoteproc/arm_rproc.c
> @@ -0,0 +1,395 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Arm Limited and/or its affiliates <open-source-office@arm.com>
> + *
> + * Authors:
> + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/firmware.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +/**
> + * struct arm_rproc_reset_cfg - remote processor reset configuration
> + * @ctrl_reg: address of the control register
> + * @state_reg: address of the reset status register
> + */
> +struct arm_rproc_reset_cfg {
> + void __iomem *ctrl_reg;
> + void __iomem *state_reg;
> +};
> +
> +struct arm_rproc;
> +
This is useless, please remove.
> +/**
> + * struct arm_rproc_dcfg - Arm remote processor configuration
Configuration? This looks more like operations to me. Please consider
renaming.
> + * @stop: stop callback function
> + * @start: start callback function
> + */
> +struct arm_rproc_dcfg {
> + int (*stop)(struct rproc *rproc);
> + int (*start)(struct rproc *rproc);
> +};
> +
> +/**
> + * struct arm_rproc - Arm remote processor instance
> + * @rproc: rproc handler
> + * @core_dcfg: device configuration pointer
> + * @reset_cfg: reset configuration registers
> + */
> +struct arm_rproc {
> + struct rproc *rproc;
> + const struct arm_rproc_dcfg *core_dcfg;
> + struct arm_rproc_reset_cfg reset_cfg;
> +};
> +
> +/* Definitions for Arm Corstone-1000 External System */
> +
> +#define EXTSYS_RST_CTRL_CPUWAIT BIT(0)
> +#define EXTSYS_RST_CTRL_RST_REQ BIT(1)
> +
> +#define EXTSYS_RST_ACK_MASK GENMASK(2, 1)
> +#define EXTSYS_RST_ST_RST_ACK(x) \
> + ((u8)(FIELD_GET(EXTSYS_RST_ACK_MASK, (x))))
> +
> +#define EXTSYS_RST_ACK_NO_RESET_REQ (0x0)
> +#define EXTSYS_RST_ACK_NOT_COMPLETE (0x1)
> +#define EXTSYS_RST_ACK_COMPLETE (0x2)
> +#define EXTSYS_RST_ACK_RESERVED (0x3)
> +
> +#define EXTSYS_RST_ACK_POLL_TRIES (3)
> +#define EXTSYS_RST_ACK_POLL_TIMEOUT (1000)
On my side most of the values above came out misaligned.
> +
> +/**
> + * arm_rproc_start_cs1000_extsys() - custom start function
> + * @rproc: pointer to the remote processor object
> + *
> + * Start function for Corstone-1000 External System.
> + * Allow the External System core start execute instructions.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_start_cs1000_extsys(struct rproc *rproc)
> +{
> + struct arm_rproc *priv = rproc->priv;
> + u32 ctrl_reg;
> +
> + /* CPUWAIT signal of the External System is de-asserted */
> + ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
> + ctrl_reg &= ~EXTSYS_RST_CTRL_CPUWAIT;
> + writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
> +
> + return 0;
> +}
> +
> +/**
> + * arm_rproc_cs1000_extsys_poll_rst_ack() - poll RST_ACK bits
> + * @rproc: pointer to the remote processor object
> + * @exp_ack: expected bits value
> + * @rst_ack: bits value read
> + *
> + * Tries to read RST_ACK bits until the timeout expires.
> + * EXTSYS_RST_ACK_POLL_TRIES tries are made,
> + * every EXTSYS_RST_ACK_POLL_TIMEOUT milliseconds.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_cs1000_extsys_poll_rst_ack(struct rproc *rproc,
> + u8 exp_ack, u8 *rst_ack)
> +{
> + struct arm_rproc *priv = rproc->priv;
> + struct device *dev = rproc->dev.parent;
> + u32 state_reg;
> + int tries = EXTSYS_RST_ACK_POLL_TRIES;
> + unsigned long timeout;
> +
struct device *dev = rproc->dev.parent;
struct arm_rproc *priv = rproc->priv;
int tries = EXTSYS_RST_ACK_POLL_TRIES;
unsigned long timeout;
u32 state_reg;
> + do {
> + state_reg = readl(priv->reset_cfg.state_reg);
> + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> +
> + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> + *rst_ack);
> + return -EINVAL;
> + }
> +
> + /* expected ACK value read */
> + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
I'm not sure why the second condition in this if() statement is needed. As far
as I can tell the first condition will trigger and the second one won't be
reached.
> + return 0;
> +
> + timeout = msleep_interruptible(EXTSYS_RST_ACK_POLL_TIMEOUT);
> +
> + if (timeout) {
> + dev_err(dev, "polling RST_ACK aborted\n");
> + return -ECONNABORTED;
> + }
> + } while (--tries);
> +
> + dev_err(dev, "polling RST_ACK timed out\n");
> +
> + return -ETIMEDOUT;
> +}
> +
> +/**
> + * arm_rproc_stop_cs1000_extsys() - custom stop function
> + * @rproc: pointer to the remote processor object
> + *
> + * Reset all logic within the External System, the core will be in a halt state.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_stop_cs1000_extsys(struct rproc *rproc)
> +{
> + struct arm_rproc *priv = rproc->priv;
> + struct device *dev = rproc->dev.parent;
> + u32 ctrl_reg;
> + u8 rst_ack, req_status;
> + int ret;
> +
struct device *dev = rproc->dev.parent;
struct arm_rproc *priv = rproc->priv;
u8 rst_ack, req_status;
u32 ctrl_reg;
int ret;
> + ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
> + ctrl_reg |= EXTSYS_RST_CTRL_RST_REQ;
> + writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
> +
> + ret = arm_rproc_cs1000_extsys_poll_rst_ack(rproc,
> + EXTSYS_RST_ACK_COMPLETE |
> + EXTSYS_RST_ACK_NOT_COMPLETE,
> + &rst_ack);
> + if (ret)
> + return ret;
> +
> + req_status = rst_ack;
> +
> + ctrl_reg = readl(priv->reset_cfg.ctrl_reg);
> + ctrl_reg &= ~EXTSYS_RST_CTRL_RST_REQ;
> + writel(ctrl_reg, priv->reset_cfg.ctrl_reg);
> +
> + ret = arm_rproc_cs1000_extsys_poll_rst_ack(rproc, 0, &rst_ack);
> + if (ret)
> + return ret;
> +
> + if (req_status == EXTSYS_RST_ACK_COMPLETE) {
> + dev_dbg(dev, "the requested reset has been accepted\n");
Please remove
> + return 0;
> + }
> +
> + dev_err(dev, "the requested reset has been denied\n");
There is enough error reporting in arm_rproc_cs1000_extsys_poll_rst_ack(),
please remove.
> + return -EACCES;
> +}
> +
> +static const struct arm_rproc_dcfg arm_rproc_cfg_corstone1000_extsys = {
> + .stop = arm_rproc_stop_cs1000_extsys,
> + .start = arm_rproc_start_cs1000_extsys,
> +};
> +
> +/**
> + * arm_rproc_stop() - Stop function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + *
> + * Calls the stop() callback of the remote core
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_stop(struct rproc *rproc)
> +{
> + struct arm_rproc *priv = rproc->priv;
> +
> + return priv->core_dcfg->stop(rproc);
> +}
> +
> +/**
> + * arm_rproc_start() - Start function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + *
> + * Calls the start() callback of the remote core
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_start(struct rproc *rproc)
> +{
> + struct arm_rproc *priv = rproc->priv;
> +
> + return priv->core_dcfg->start(rproc);
> +}
> +
> +/**
> + * arm_rproc_parse_fw() - Parse firmware function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + * @fw: pointer to the firmware
> + *
> + * Does nothing currently.
> + *
> + * Return:
> + *
> + * 0 for success.
> + */
> +static int arm_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> + return 0;
> +}
> +
> +/**
> + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> + * @rproc: pointer to the remote processor object
> + * @fw: pointer to the firmware
> + *
> + * Does nothing currently.
> + *
> + * Return:
> + *
> + * 0 for success.
> + */
> +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> +{
What is the point of doing rproc_of_parse_firmware() if the firmware image is
not loaded to memory? Does the remote processor have some kind of default ROM
image to run if it doesn't find anything in memory?
> + return 0;
> +}
> +
> +static const struct rproc_ops arm_rproc_ops = {
> + .start = arm_rproc_start,
> + .stop = arm_rproc_stop,
> + .load = arm_rproc_load,
> + .parse_fw = arm_rproc_parse_fw,
> +};
> +
> +/**
> + * arm_rproc_probe() - the platform device probe
> + * @pdev: the platform device
> + *
> + * Read from the device tree the properties needed to setup
> + * the reset and comms for the remote processor.
> + * Also, allocate a rproc device and register it with the remoteproc subsystem.
> + *
> + * Return:
> + *
> + * 0 on success. Otherwise, failure
> + */
> +static int arm_rproc_probe(struct platform_device *pdev)
> +{
> + const struct arm_rproc_dcfg *core_dcfg;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct arm_rproc *priv;
> + struct rproc *rproc;
> + const char *fw_name;
> + int ret;
> + struct resource *res;
> +
> + core_dcfg = of_device_get_match_data(dev);
> + if (!core_dcfg)
> + return -ENODEV;
> +
> + ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> + if (ret) {
> + dev_err(dev,
> + "can't parse firmware-name from device tree (%pe)\n",
> + ERR_PTR(ret));
> + return ret;
Please replace with dev_err_probe().
> + }
> +
> + dev_dbg(dev, "firmware-name: %s\n", fw_name);
> +
> + rproc = rproc_alloc(dev, np->name, &arm_rproc_ops, fw_name,
> + sizeof(*priv));
Using devm_rproc_alloc() would make this driver even more simple.
> + if (!rproc)
> + return -ENOMEM;
> +
> + priv = rproc->priv;
> + priv->rproc = rproc;
> + priv->core_dcfg = core_dcfg;
> +
> + res = platform_get_resource_byname(pdev,
> + IORESOURCE_MEM, "reset-control");
> + priv->reset_cfg.ctrl_reg = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv->reset_cfg.ctrl_reg)) {
> + ret = PTR_ERR(priv->reset_cfg.ctrl_reg);
> + dev_err(dev,
> + "can't map the reset-control register (%pe)\n",
> + ERR_PTR((unsigned long)priv->reset_cfg.ctrl_reg));
dev_err_probe()
> + goto err_free_rproc;
This isn't needed after moving to devm_rproc_alloc().
> + } else {
> + dev_dbg(dev, "reset-control: %p\n", priv->reset_cfg.ctrl_reg);
> + }
> +
> + res = platform_get_resource_byname(pdev,
> + IORESOURCE_MEM, "reset-status");
> + priv->reset_cfg.state_reg = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv->reset_cfg.state_reg)) {
> + ret = PTR_ERR(priv->reset_cfg.state_reg);
> + dev_err(dev,
> + "can't map the reset-status register (%pe)\n",
> + ERR_PTR((unsigned long)priv->reset_cfg.state_reg));
> + goto err_free_rproc;
Same comments as above.
> + } else {
> + dev_dbg(dev, "reset-status: %p\n",
> + priv->reset_cfg.state_reg);
This isn't adding anything valuable, please remove.
> + }
> +
> + platform_set_drvdata(pdev, rproc);
> +
> + ret = rproc_add(rproc);
devm_rproc_add()
> + if (ret) {
> + dev_err(dev, "can't add remote processor (%pe)\n",
> + ERR_PTR(ret));
> + goto err_free_rproc;
> + } else {
> + dev_dbg(dev, "remote processor added\n");
Please remove.
> + }
> +
> + return 0;
> +
> +err_free_rproc:
> + rproc_free(rproc);
> +
> + return ret;
> +}
> +
> +/**
> + * arm_rproc_remove() - the platform device remove
> + * @pdev: the platform device
> + *
> + * Delete and free the resources used.
> + */
> +static void arm_rproc_remove(struct platform_device *pdev)
> +{
> + struct rproc *rproc = platform_get_drvdata(pdev);
> +
> + rproc_del(rproc);
> + rproc_free(rproc);
> +}
This shouldn't be needed anymore after using devm_rproc_alloc() and
devm_rproc_add().
> +
> +static const struct of_device_id arm_rproc_of_match[] = {
> + { .compatible = "arm,corstone1000-extsys", .data = &arm_rproc_cfg_corstone1000_extsys },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, arm_rproc_of_match);
> +
> +static struct platform_driver arm_rproc_driver = {
> + .probe = arm_rproc_probe,
> + .remove_new = arm_rproc_remove,
> + .driver = {
> + .name = "arm-rproc",
> + .of_match_table = arm_rproc_of_match,
> + },
> +};
> +module_platform_driver(arm_rproc_driver);
> +
I am echoing Krzysztof view about how generic this driver name is. This has to
be related to a family of processors or be made less generic in some way. Have
a look at what TI did for their K3 lineup [1] - I would like to see the same
thing done here.
Thanks,
Mathieu
[1]. https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c#L898
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Arm Remote Processor Control Driver");
> +MODULE_AUTHOR("Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>");
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-04 18:42 ` Mathieu Poirier
@ 2024-03-07 19:40 ` Abdellatif El Khlifi
2024-03-08 16:44 ` Mathieu Poirier
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-07 19:40 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
Hi Mathieu,
> > + do {
> > + state_reg = readl(priv->reset_cfg.state_reg);
> > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > +
> > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > + *rst_ack);
> > + return -EINVAL;
> > + }
> > +
> > + /* expected ACK value read */
> > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
>
> I'm not sure why the second condition in this if() statement is needed. As far
> as I can tell the first condition will trigger and the second one won't be
> reached.
The second condition takes care of the following: exp_ack and *rst_ack are both 0.
This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
we expect the RST_ACK to be 00 afterwards.
> > +/**
> > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > + * @rproc: pointer to the remote processor object
> > + * @fw: pointer to the firmware
> > + *
> > + * Does nothing currently.
> > + *
> > + * Return:
> > + *
> > + * 0 for success.
> > + */
> > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > +{
>
> What is the point of doing rproc_of_parse_firmware() if the firmware image is
> not loaded to memory? Does the remote processor have some kind of default ROM
> image to run if it doesn't find anything in memory?
Yes, the remote processor has a default FW image already loaded by default.
rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
provided.
Please correct me if I'm wrong.
[1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
[2]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863
> > +module_platform_driver(arm_rproc_driver);
> > +
>
> I am echoing Krzysztof view about how generic this driver name is. This has to
> be related to a family of processors or be made less generic in some way. Have
> a look at what TI did for their K3 lineup [1] - I would like to see the same
> thing done here.
Thank you, I'll take care of that and of all the other comments made.
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/3] arm64: dts: Add corstone1000 external system device node
2024-03-01 16:42 ` [PATCH 2/3] arm64: dts: Add corstone1000 external system device node abdellatif.elkhlifi
2024-03-01 19:27 ` Krzysztof Kozlowski
@ 2024-03-08 12:21 ` Sudeep Holla
2024-03-08 14:25 ` Abdellatif El Khlifi
1 sibling, 1 reply; 59+ messages in thread
From: Sudeep Holla @ 2024-03-08 12:21 UTC (permalink / raw)
To: abdellatif.elkhlifi
Cc: Bjorn Andersson, Mathieu Poirier, Sudeep Holla, Rob Herring,
Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley,
Drew.Reed, Adam.Johnston, linux-arm-kernel, devicetree,
linux-kernel, linux-remoteproc
On Fri, Mar 01, 2024 at 04:42:26PM +0000, abdellatif.elkhlifi@arm.com wrote:
> From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>
> add device tree node for the external system core in Corstone-1000
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> arch/arm64/boot/dts/arm/corstone1000.dtsi | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/arm/corstone1000.dtsi b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> index 6ad7829f9e28..67df642363e9 100644
> --- a/arch/arm64/boot/dts/arm/corstone1000.dtsi
> +++ b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0 OR MIT
> /*
> - * Copyright (c) 2022, Arm Limited. All rights reserved.
> + * Copyright 2022, 2024, Arm Limited and/or its affiliates <open-source-office@arm.com>
> * Copyright (c) 2022, Linaro Limited. All rights reserved.
> *
> */
> @@ -157,5 +157,13 @@ mhu_seh1: mailbox@1b830000 {
> secure-status = "okay"; /* secure-world-only */
> status = "disabled";
> };
> +
> + extsys0: remoteproc@1a010310 {
> + compatible = "arm,corstone1000-extsys";
> + reg = <0x1a010310 0x4>,
> + <0x1a010314 0X4>;
As per [1], this is just a few registers within the 64kB block.
Not sure if it should be represented as a whole on just couple
of registers like this for reset.
--
Regards,
Sudeep
[1] https://developer.arm.com/documentation/101418/0100/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-01 19:30 ` Krzysztof Kozlowski
@ 2024-03-08 12:29 ` Sudeep Holla
2024-03-08 13:54 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Sudeep Holla @ 2024-03-08 12:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: abdellatif.elkhlifi, Bjorn Andersson, Mathieu Poirier,
Sudeep Holla, Rob Herring, Liviu Dudau, Lorenzo Pieralisi,
Krzysztof Kozlowski, Conor Dooley, Drew.Reed, Adam.Johnston,
linux-arm-kernel, devicetree, linux-kernel, linux-remoteproc
On Fri, Mar 01, 2024 at 08:30:43PM +0100, Krzysztof Kozlowski wrote:
> On 01/03/2024 17:42, abdellatif.elkhlifi@arm.com wrote:
> > From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> >
> > introduce the bindings for Arm remoteproc support.
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > ---
> > .../bindings/remoteproc/arm,rproc.yaml | 69 +++++++++++++++++++
> > MAINTAINERS | 1 +
>
> Fix order of patches - bindings are always before the user (see
> submitting bindings doc).
>
> > 2 files changed, 70 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> > new file mode 100644
> > index 000000000000..322197158059
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/remoteproc/arm,rproc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Arm Remoteproc Devices
>
> That's quite generic... does it applied to all ARM designs?
>
Nope, it is platform specific. It can't just generically be referred as
Arm Remoteproc for sure.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-08 12:29 ` Sudeep Holla
@ 2024-03-08 13:54 ` Abdellatif El Khlifi
0 siblings, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-08 13:54 UTC (permalink / raw)
To: Sudeep Holla, Krzysztof Kozlowski
Cc: Bjorn Andersson, Mathieu Poirier, Rob Herring, Liviu Dudau,
Lorenzo Pieralisi, Conor Dooley, Drew.Reed, Adam.Johnston,
linux-arm-kernel, devicetree, linux-kernel, linux-remoteproc
Hi Krzysztof, Sudeep,
> > > diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> > > new file mode 100644
> > > index 000000000000..322197158059
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> > > @@ -0,0 +1,69 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/remoteproc/arm,rproc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Arm Remoteproc Devices
> >
> > That's quite generic... does it applied to all ARM designs?
> >
>
> Nope, it is platform specific. It can't just generically be referred as
> Arm Remoteproc for sure.
Thank you guys.
The file names and the documentation will reflect that it's
an Arm Corstone SoC. Work in progress.
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/3] arm64: dts: Add corstone1000 external system device node
2024-03-08 12:21 ` Sudeep Holla
@ 2024-03-08 14:25 ` Abdellatif El Khlifi
0 siblings, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-08 14:25 UTC (permalink / raw)
To: Sudeep Holla
Cc: Bjorn Andersson, Mathieu Poirier, Rob Herring, Liviu Dudau,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
Hi Sudeep,
> > + extsys0: remoteproc@1a010310 {
> > + compatible = "arm,corstone1000-extsys";
> > + reg = <0x1a010310 0x4>,
> > + <0x1a010314 0X4>;
>
>
> As per [1], this is just a few registers within the 64kB block.
> Not sure if it should be represented as a whole on just couple
> of registers like this for reset.
>
> [1] https://developer.arm.com/documentation/101418/0100/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
The Host Base System Control registers are not specific to the External System processors. They are various registers with different purposes.
Only 4 registers matter for the remoteproc feature:
- The External system 0 reset control and status registers: EXT_SYS0_RST_CTRL, EXT_SYS0_RST_ST
- Same for the the External system 1: EXT_SYS1_RST_CTRL, EXT_SYS1_RST_ST
So, mapping the whole Host Base System Control area doesn't make sense for the remoteproc feature
and exposes registers that are not related to the External Systems to the driver.
By the way, the latest document we are referring to is [1].
[1]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-07 19:40 ` Abdellatif El Khlifi
@ 2024-03-08 16:44 ` Mathieu Poirier
2024-03-11 11:44 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Mathieu Poirier @ 2024-03-08 16:44 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
<abdellatif.elkhlifi@arm.com> wrote:
>
> Hi Mathieu,
>
> > > + do {
> > > + state_reg = readl(priv->reset_cfg.state_reg);
> > > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > > +
> > > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > > + *rst_ack);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* expected ACK value read */
> > > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
> >
> > I'm not sure why the second condition in this if() statement is needed. As far
> > as I can tell the first condition will trigger and the second one won't be
> > reached.
>
> The second condition takes care of the following: exp_ack and *rst_ack are both 0.
> This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
> we expect the RST_ACK to be 00 afterwards.
>
This is the kind of conditions that definitely deserve documentation.
Please split the conditions in two different if() statements and add a
comment to explain what is going on.
> > > +/**
> > > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > > + * @rproc: pointer to the remote processor object
> > > + * @fw: pointer to the firmware
> > > + *
> > > + * Does nothing currently.
> > > + *
> > > + * Return:
> > > + *
> > > + * 0 for success.
> > > + */
> > > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > > +{
> >
> > What is the point of doing rproc_of_parse_firmware() if the firmware image is
> > not loaded to memory? Does the remote processor have some kind of default ROM
> > image to run if it doesn't find anything in memory?
>
> Yes, the remote processor has a default FW image already loaded by default.
>
That too would have mandated a comment - otherwise people looking at
the code are left wondering, as I did.
> rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
> provided.
>
> Please correct me if I'm wrong.
You are correct, the remoteproc subsystem expects a firmware image to
be provided _and_ loaded into memory. Providing a dummy image just to
get the remote processor booted is a hack, but simply because the
subsystem isn't tailored to handle this use case. So I am left
wondering what the plans are for this driver, i.e is this a real
scenario that needs to be addressed or just an initial patchset to get
a foundation for the driver.
In the former case we need to start talking about refactoring the
subsystem so that it properly handles remote processors that don't
need a firmware image. In the latter case I'd rather see a patchset
where the firmware image is loaded into RAM.
>
> [1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
> [2]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863
>
> > > +module_platform_driver(arm_rproc_driver);
> > > +
> >
> > I am echoing Krzysztof view about how generic this driver name is. This has to
> > be related to a family of processors or be made less generic in some way. Have
> > a look at what TI did for their K3 lineup [1] - I would like to see the same
> > thing done here.
>
> Thank you, I'll take care of that and of all the other comments made.
>
> Cheers,
> Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-08 16:44 ` Mathieu Poirier
@ 2024-03-11 11:44 ` Abdellatif El Khlifi
2024-03-12 16:29 ` Mathieu Poirier
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-11 11:44 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
Hi Mathieu,
On Fri, Mar 08, 2024 at 09:44:26AM -0700, Mathieu Poirier wrote:
> On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
> <abdellatif.elkhlifi@arm.com> wrote:
> >
> > Hi Mathieu,
> >
> > > > + do {
> > > > + state_reg = readl(priv->reset_cfg.state_reg);
> > > > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > > > +
> > > > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > > > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > > > + *rst_ack);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + /* expected ACK value read */
> > > > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
> > >
> > > I'm not sure why the second condition in this if() statement is needed. As far
> > > as I can tell the first condition will trigger and the second one won't be
> > > reached.
> >
> > The second condition takes care of the following: exp_ack and *rst_ack are both 0.
> > This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
> > we expect the RST_ACK to be 00 afterwards.
> >
>
> This is the kind of conditions that definitely deserve documentation.
> Please split the conditions in two different if() statements and add a
> comment to explain what is going on.
Thanks, I'll address that.
>
> > > > +/**
> > > > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > > > + * @rproc: pointer to the remote processor object
> > > > + * @fw: pointer to the firmware
> > > > + *
> > > > + * Does nothing currently.
> > > > + *
> > > > + * Return:
> > > > + *
> > > > + * 0 for success.
> > > > + */
> > > > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > > > +{
> > >
> > > What is the point of doing rproc_of_parse_firmware() if the firmware image is
> > > not loaded to memory? Does the remote processor have some kind of default ROM
> > > image to run if it doesn't find anything in memory?
> >
> > Yes, the remote processor has a default FW image already loaded by default.
> >
>
> That too would have mandated a comment - otherwise people looking at
> the code are left wondering, as I did.
>
> > rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
> > provided.
> >
> > Please correct me if I'm wrong.
>
> You are correct, the remoteproc subsystem expects a firmware image to
> be provided _and_ loaded into memory. Providing a dummy image just to
> get the remote processor booted is a hack, but simply because the
> subsystem isn't tailored to handle this use case. So I am left
> wondering what the plans are for this driver, i.e is this a real
> scenario that needs to be addressed or just an initial patchset to get
> a foundation for the driver.
>
> In the former case we need to start talking about refactoring the
> subsystem so that it properly handles remote processors that don't
> need a firmware image. In the latter case I'd rather see a patchset
> where the firmware image is loaded into RAM.
This is an initial patchset for allowing to turn on and off the remote processor.
The FW is already loaded before the Corstone-1000 SoC is powered on and this
is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
(emulator).
The plan for the driver is as follows:
Step 1: provide a foundation driver capable of turning the core on/off
Step 2: provide mailbox support for comms
Step 3: provide FW reload capability
Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
the remote core.
I'm happy to provide more explanation in the commit log to reflect this status.
Is it OK that we go with step 1 as a foundation please ?
Cheers
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-11 11:44 ` Abdellatif El Khlifi
@ 2024-03-12 16:29 ` Mathieu Poirier
2024-03-12 17:32 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Mathieu Poirier @ 2024-03-12 16:29 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
On Mon, 11 Mar 2024 at 05:44, Abdellatif El Khlifi
<abdellatif.elkhlifi@arm.com> wrote:
>
> Hi Mathieu,
>
> On Fri, Mar 08, 2024 at 09:44:26AM -0700, Mathieu Poirier wrote:
> > On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
> > <abdellatif.elkhlifi@arm.com> wrote:
> > >
> > > Hi Mathieu,
> > >
> > > > > + do {
> > > > > + state_reg = readl(priv->reset_cfg.state_reg);
> > > > > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > > > > +
> > > > > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > > > > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > > > > + *rst_ack);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + /* expected ACK value read */
> > > > > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
> > > >
> > > > I'm not sure why the second condition in this if() statement is needed. As far
> > > > as I can tell the first condition will trigger and the second one won't be
> > > > reached.
> > >
> > > The second condition takes care of the following: exp_ack and *rst_ack are both 0.
> > > This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
> > > we expect the RST_ACK to be 00 afterwards.
> > >
> >
> > This is the kind of conditions that definitely deserve documentation.
> > Please split the conditions in two different if() statements and add a
> > comment to explain what is going on.
>
> Thanks, I'll address that.
>
> >
> > > > > +/**
> > > > > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > > > > + * @rproc: pointer to the remote processor object
> > > > > + * @fw: pointer to the firmware
> > > > > + *
> > > > > + * Does nothing currently.
> > > > > + *
> > > > > + * Return:
> > > > > + *
> > > > > + * 0 for success.
> > > > > + */
> > > > > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > > > > +{
> > > >
> > > > What is the point of doing rproc_of_parse_firmware() if the firmware image is
> > > > not loaded to memory? Does the remote processor have some kind of default ROM
> > > > image to run if it doesn't find anything in memory?
> > >
> > > Yes, the remote processor has a default FW image already loaded by default.
> > >
> >
> > That too would have mandated a comment - otherwise people looking at
> > the code are left wondering, as I did.
> >
> > > rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
> > > provided.
> > >
> > > Please correct me if I'm wrong.
> >
> > You are correct, the remoteproc subsystem expects a firmware image to
> > be provided _and_ loaded into memory. Providing a dummy image just to
> > get the remote processor booted is a hack, but simply because the
> > subsystem isn't tailored to handle this use case. So I am left
> > wondering what the plans are for this driver, i.e is this a real
> > scenario that needs to be addressed or just an initial patchset to get
> > a foundation for the driver.
> >
> > In the former case we need to start talking about refactoring the
> > subsystem so that it properly handles remote processors that don't
> > need a firmware image. In the latter case I'd rather see a patchset
> > where the firmware image is loaded into RAM.
>
> This is an initial patchset for allowing to turn on and off the remote processor.
> The FW is already loaded before the Corstone-1000 SoC is powered on and this
> is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> (emulator).
>
From the above I take it that booting with a preloaded firmware is a
scenario that needs to be supported and not just a temporary stage.
> The plan for the driver is as follows:
>
> Step 1: provide a foundation driver capable of turning the core on/off
>
> Step 2: provide mailbox support for comms
>
> Step 3: provide FW reload capability
>
What happens when a user wants to boot the remote processor with the
firmware provided on the file system rather than the one preloaded
into memory? Furthermore, how do we account for scenarios where the
remote processor goes from running a firmware image on the file system
to the firmware image loaded by an external entity? Is this a valid
scenario?
> Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
> the remote core.
>
> I'm happy to provide more explanation in the commit log to reflect this status.
>
> Is it OK that we go with step 1 as a foundation please ?
>
First let's clarify all the scenarios that need to be supported. From
there I will advise on how to proceed and what modifications to the
subsystem's core should be made, if need be.
> Cheers
> Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-12 16:29 ` Mathieu Poirier
@ 2024-03-12 17:32 ` Abdellatif El Khlifi
2024-03-13 16:25 ` Mathieu Poirier
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-12 17:32 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
Hi Mathieu,
On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > This is an initial patchset for allowing to turn on and off the remote processor.
> > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > (emulator).
> >
> >From the above I take it that booting with a preloaded firmware is a
> scenario that needs to be supported and not just a temporary stage.
The current status of the Corstone-1000 SoC requires that there is
a preloaded firmware for the external core. Preloading is done externally
either through the FPGA bootloader or the emulator (FVP) before powering
on the SoC.
Corstone-1000 will be upgraded in a way that the A core running Linux is able
to share memory with the remote core and also being able to access the remote
core memory so Linux can copy the firmware to. This HW changes are still
under development.
This is why this patchset is relying on a preloaded firmware. And it's the step 1
of adding remoteproc support for Corstone.
When the HW is ready, we will be able to avoid preloading the firmware
and the user can do the following:
1) Use a default firmware filename stated in the DT (firmware-name property),
that's the one remoteproc subsystem will use initially, load the firmware file
and start the remote core.
2) Then, the user can choose to use another firmware file:
echo stop >/sys/class/remoteproc/remoteproc0/state
echo -n new_firmware.elf > /sys/class/remoteproc/remoteproc0/firmware
echo start >/sys/class/remoteproc/remoteproc0/state
> > The plan for the driver is as follows:
> >
> > Step 1: provide a foundation driver capable of turning the core on/off
> >
> > Step 2: provide mailbox support for comms
> >
> > Step 3: provide FW reload capability
> >
> What happens when a user wants to boot the remote processor with the
> firmware provided on the file system rather than the one preloaded
> into memory?
We will support this scenario when the HW is upgraded and copying the firmware
to the remote core memory becomes possible.
> Furthermore, how do we account for scenarios where the
> remote processor goes from running a firmware image on the file system
> to the firmware image loaded by an external entity? Is this a valid
> scenario?
No, this scenario won't apply when we get the HW upgrade. No need for an
external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
> > the remote core.
> >
> > I'm happy to provide more explanation in the commit log to reflect this status.
> >
> > Is it OK that we go with step 1 as a foundation please ?
> >
>
> First let's clarify all the scenarios that need to be supported. From
> there I will advise on how to proceed and what modifications to the
> subsystem's core should be made, if need be.
Thanks, I hope the answers above provide the information needed.
Cheers
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-12 17:32 ` Abdellatif El Khlifi
@ 2024-03-13 16:25 ` Mathieu Poirier
2024-03-13 17:17 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Mathieu Poirier @ 2024-03-13 16:25 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> Hi Mathieu,
>
> On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > (emulator).
> > >
> > >From the above I take it that booting with a preloaded firmware is a
> > scenario that needs to be supported and not just a temporary stage.
>
> The current status of the Corstone-1000 SoC requires that there is
> a preloaded firmware for the external core. Preloading is done externally
> either through the FPGA bootloader or the emulator (FVP) before powering
> on the SoC.
>
Ok
> Corstone-1000 will be upgraded in a way that the A core running Linux is able
> to share memory with the remote core and also being able to access the remote
> core memory so Linux can copy the firmware to. This HW changes are still
> This is why this patchset is relying on a preloaded firmware. And it's the step 1
> of adding remoteproc support for Corstone.
>
Ok, so there is a HW problem where A core and M core can't see each other's
memory, preventing the A core from copying the firmware image to the proper
location.
When the HW is fixed, will there be a need to support scenarios where the
firmware image has been preloaded into memory?
> When the HW is ready, we will be able to avoid preloading the firmware
> and the user can do the following:
>
> 1) Use a default firmware filename stated in the DT (firmware-name property),
> that's the one remoteproc subsystem will use initially, load the firmware file
> and start the remote core.
>
> 2) Then, the user can choose to use another firmware file:
>
> echo stop >/sys/class/remoteproc/remoteproc0/state
> echo -n new_firmware.elf > /sys/class/remoteproc/remoteproc0/firmware
> echo start >/sys/class/remoteproc/remoteproc0/state
>
> > > The plan for the driver is as follows:
> > >
> > > Step 1: provide a foundation driver capable of turning the core on/off
> > >
> > > Step 2: provide mailbox support for comms
> > >
> > > Step 3: provide FW reload capability
> > >
> > What happens when a user wants to boot the remote processor with the
> > firmware provided on the file system rather than the one preloaded
> > into memory?
>
> We will support this scenario when the HW is upgraded and copying the firmware
> to the remote core memory becomes possible.
>
> > Furthermore, how do we account for scenarios where the
> > remote processor goes from running a firmware image on the file system
> > to the firmware image loaded by an external entity? Is this a valid
> > scenario?
>
> No, this scenario won't apply when we get the HW upgrade. No need for an
> external entity anymore. The firmware(s) will all be files in the linux filesystem.
>
> > > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can share memory with
> > > the remote core.
> > >
> > > I'm happy to provide more explanation in the commit log to reflect this status.
> > >
> > > Is it OK that we go with step 1 as a foundation please ?
> > >
> >
> > First let's clarify all the scenarios that need to be supported. From
> > there I will advise on how to proceed and what modifications to the
> > subsystem's core should be made, if need be.
>
> Thanks, I hope the answers above provide the information needed.
>
> Cheers
> Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-13 16:25 ` Mathieu Poirier
@ 2024-03-13 17:17 ` Abdellatif El Khlifi
2024-03-14 14:52 ` Mathieu Poirier
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-13 17:17 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
Hi Mathieu,
On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > Hi Mathieu,
> >
> > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > (emulator).
> > > >
> > > >From the above I take it that booting with a preloaded firmware is a
> > > scenario that needs to be supported and not just a temporary stage.
> >
> > The current status of the Corstone-1000 SoC requires that there is
> > a preloaded firmware for the external core. Preloading is done externally
> > either through the FPGA bootloader or the emulator (FVP) before powering
> > on the SoC.
> >
>
> Ok
>
> > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > to share memory with the remote core and also being able to access the remote
> > core memory so Linux can copy the firmware to. This HW changes are still
> > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > of adding remoteproc support for Corstone.
> >
>
> Ok, so there is a HW problem where A core and M core can't see each other's
> memory, preventing the A core from copying the firmware image to the proper
> location.
>
> When the HW is fixed, will there be a need to support scenarios where the
> firmware image has been preloaded into memory?
No, this scenario won't apply when we get the HW upgrade. No need for an
external entity anymore. The firmware(s) will all be files in the linux filesystem.
Cheers
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-01 16:42 ` [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc abdellatif.elkhlifi
2024-03-01 19:30 ` Krzysztof Kozlowski
@ 2024-03-13 19:59 ` Robin Murphy
2024-03-14 13:49 ` Abdellatif El Khlifi
1 sibling, 1 reply; 59+ messages in thread
From: Robin Murphy @ 2024-03-13 19:59 UTC (permalink / raw)
To: abdellatif.elkhlifi, Bjorn Andersson, Mathieu Poirier,
Rob Herring
Cc: Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
On 2024-03-01 4:42 pm, abdellatif.elkhlifi@arm.com wrote:
> From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>
> introduce the bindings for Arm remoteproc support.
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> .../bindings/remoteproc/arm,rproc.yaml | 69 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 70 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> new file mode 100644
> index 000000000000..322197158059
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/arm,rproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Arm Remoteproc Devices
> +
> +maintainers:
> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> +
> +description: |
> + Some Arm heterogeneous System-On-Chips feature remote processors that can
> + be controlled with a reset control register and a reset status register to
> + start or stop the processor.
> +
> + This document defines the bindings for these remote processors.
> +
> +properties:
> + compatible:
> + enum:
> + - arm,corstone1000-extsys
> +
> + reg:
> + minItems: 2
> + maxItems: 2
> + description: |
> + Address and size in bytes of the reset control register
> + and the reset status register.
> + Expects the registers to be in the order as above.
> + Should contain an entry for each value in 'reg-names'.
> +
> + reg-names:
> + description: |
> + Required names for each of the reset registers defined in
> + the 'reg' property. Expects the names from the following
> + list, in the specified order, each representing the corresponding
> + reset register.
> + items:
> + - const: reset-control
> + - const: reset-status
> +
> + firmware-name:
> + description: |
> + Default name of the firmware to load to the remote processor.
So... is loading the firmware image achieved by somehow bitbanging it
through the one reset register, maybe? I find it hard to believe this is
a complete and functional binding.
Frankly at the moment I'd be inclined to say it isn't even a remoteproc
binding (or driver) at all, it's a reset controller. Bindings are a
contract for describing the hardware, not the current state of Linux
driver support - if this thing still needs mailboxes, shared memory, a
reset vector register, or whatever else to actually be useful, those
should be in the binding from day 1 so that a) people can write and
deploy correct DTs now, such that functionality becomes available on
their systems as soon as driver support catches up, and b) the community
has any hope of being able to review whether the binding is
appropriately designed and specified for the purpose it intends to serve.
For instance right now it seems somewhat tenuous to describe two
consecutive 32-bit registers as separate "reg" entries, but *maybe* it's
OK if that's all there ever is. However if it's actually going to end up
needing several more additional MMIO and/or memory regions for other
functionality, then describing each register and location individually
is liable to get unmanageable really fast, and a higher-level functional
grouping (e.g. these reset-related registers together as a single 8-byte
region) would likely be a better design.
Thanks,
Robin.
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> + - firmware-name
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + extsys0: remoteproc@1a010310 {
> + compatible = "arm,corstone1000-extsys";
> + reg = <0x1a010310 0x4>, <0x1a010314 0x4>;
> + reg-names = "reset-control", "reset-status";
> + firmware-name = "es0_flashfw.elf";
> + };
> +
> + extsys1: remoteproc@1a010318 {
> + compatible = "arm,corstone1000-extsys";
> + reg = <0x1a010318 0x4>, <0x1a01031c 0x4>;
> + reg-names = "reset-control", "reset-status";
> + firmware-name = "es1_flashfw.elf";
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 54d6a40feea5..eddaa3841a65 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1768,6 +1768,7 @@ ARM REMOTEPROC DRIVER
> M: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> L: linux-remoteproc@vger.kernel.org
> S: Maintained
> +F: Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml
> F: drivers/remoteproc/arm_rproc.c
>
> ARM SMC WATCHDOG DRIVER
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-13 19:59 ` Robin Murphy
@ 2024-03-14 13:49 ` Abdellatif El Khlifi
2024-03-14 13:56 ` Krzysztof Kozlowski
2024-03-14 15:19 ` Sudeep Holla
0 siblings, 2 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-14 13:49 UTC (permalink / raw)
To: Robin Murphy
Cc: Bjorn Andersson, Mathieu Poirier, Rob Herring, Liviu Dudau,
Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
Hi Robin,
> > + firmware-name:
> > + description: |
> > + Default name of the firmware to load to the remote processor.
>
> So... is loading the firmware image achieved by somehow bitbanging it
> through the one reset register, maybe? I find it hard to believe this is a
> complete and functional binding.
>
> Frankly at the moment I'd be inclined to say it isn't even a remoteproc
> binding (or driver) at all, it's a reset controller. Bindings are a contract
> for describing the hardware, not the current state of Linux driver support -
> if this thing still needs mailboxes, shared memory, a reset vector register,
> or whatever else to actually be useful, those should be in the binding from
> day 1 so that a) people can write and deploy correct DTs now, such that
> functionality becomes available on their systems as soon as driver support
> catches up, and b) the community has any hope of being able to review
> whether the binding is appropriately designed and specified for the purpose
> it intends to serve.
This is an initial patchset for allowing to turn on and off the remote processor.
The FW is already loaded before the Corstone-1000 SoC is powered on and this
is done through the FPGA board bootloader in case of the FPGA target.
Or by the Corstone-1000 FVP model (emulator).
The plan for the driver is as follows:
Step 1: provide a foundation driver capable of turning the core on/off
Step 2: provide mailbox support for comms
Step 3: provide FW reload capability
Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can
share memory with the remote core.
So, when memory sharing becomes available in the FPGA and FVP the
DT binding will be upgraded with:
- mboxes property specifying the RX/TX mailboxes (based on MHU v2)
- memory-region property describing the virtio vrings
Currently the mailbox controller does exist in the HW but is not
usable via virtio (no memory sharing available).
Do you recommend I add the mboxes property even currently we can't do the comms ?
> For instance right now it seems somewhat tenuous to describe two consecutive
> 32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
> all there ever is. However if it's actually going to end up needing several
> more additional MMIO and/or memory regions for other functionality, then
> describing each register and location individually is liable to get
> unmanageable really fast, and a higher-level functional grouping (e.g. these
> reset-related registers together as a single 8-byte region) would likely be
> a better design.
Currently the HW provides only 2 registers to control the remote processors:
The reset control and status registers.
It makes sense to me to use a mapped region of 8 bytes for both registers rather
than individual registers (since they are consecutive).
I'll update that, thanks for the suggestion.
Abdellatif,
Cheers
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-14 13:49 ` Abdellatif El Khlifi
@ 2024-03-14 13:56 ` Krzysztof Kozlowski
2024-03-14 15:20 ` Abdellatif El Khlifi
2024-03-14 15:19 ` Sudeep Holla
1 sibling, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-14 13:56 UTC (permalink / raw)
To: Abdellatif El Khlifi, Robin Murphy
Cc: Bjorn Andersson, Mathieu Poirier, Rob Herring, Liviu Dudau,
Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
On 14/03/2024 14:49, Abdellatif El Khlifi wrote:
>> Frankly at the moment I'd be inclined to say it isn't even a remoteproc
>> binding (or driver) at all, it's a reset controller. Bindings are a contract
>> for describing the hardware, not the current state of Linux driver support -
>> if this thing still needs mailboxes, shared memory, a reset vector register,
>> or whatever else to actually be useful, those should be in the binding from
>> day 1 so that a) people can write and deploy correct DTs now, such that
>> functionality becomes available on their systems as soon as driver support
>> catches up, and b) the community has any hope of being able to review
>> whether the binding is appropriately designed and specified for the purpose
>> it intends to serve.
>
> This is an initial patchset for allowing to turn on and off the remote processor.
> The FW is already loaded before the Corstone-1000 SoC is powered on and this
> is done through the FPGA board bootloader in case of the FPGA target.
> Or by the Corstone-1000 FVP model (emulator).
>
> The plan for the driver is as follows:
>
> Step 1: provide a foundation driver capable of turning the core on/off
> Step 2: provide mailbox support for comms
> Step 3: provide FW reload capability
>
> Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can
> share memory with the remote core.
>
> So, when memory sharing becomes available in the FPGA and FVP the
> DT binding will be upgraded with:
>
> - mboxes property specifying the RX/TX mailboxes (based on MHU v2)
> - memory-region property describing the virtio vrings
>
> Currently the mailbox controller does exist in the HW but is not
> usable via virtio (no memory sharing available).
>
> Do you recommend I add the mboxes property even currently we can't do the comms ?
Bindings should be complete, regardless whether Linux driver supports it
or not. Please see writing bindings document for explanation on this and
other rules.
So yes: please describe as much as possible/reasonable.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-13 17:17 ` Abdellatif El Khlifi
@ 2024-03-14 14:52 ` Mathieu Poirier
2024-03-14 14:59 ` Sudeep Holla
0 siblings, 1 reply; 59+ messages in thread
From: Mathieu Poirier @ 2024-03-14 14:52 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: Bjorn Andersson, Rob Herring, Liviu Dudau, Sudeep Holla,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> Hi Mathieu,
>
> On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > Hi Mathieu,
> > >
> > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > (emulator).
> > > > >
> > > > >From the above I take it that booting with a preloaded firmware is a
> > > > scenario that needs to be supported and not just a temporary stage.
> > >
> > > The current status of the Corstone-1000 SoC requires that there is
> > > a preloaded firmware for the external core. Preloading is done externally
> > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > on the SoC.
> > >
> >
> > Ok
> >
> > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > to share memory with the remote core and also being able to access the remote
> > > core memory so Linux can copy the firmware to. This HW changes are still
> > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > of adding remoteproc support for Corstone.
> > >
> >
> > Ok, so there is a HW problem where A core and M core can't see each other's
> > memory, preventing the A core from copying the firmware image to the proper
> > location.
> >
> > When the HW is fixed, will there be a need to support scenarios where the
> > firmware image has been preloaded into memory?
>
> No, this scenario won't apply when we get the HW upgrade. No need for an
> external entity anymore. The firmware(s) will all be files in the linux filesystem.
>
Very well. I am willing to continue with this driver but it does so little that
I wonder if it wouldn't simply be better to move forward with upstreaming when
the HW is fixed. The choice is yours.
Thanks,
Mathieu
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-14 14:52 ` Mathieu Poirier
@ 2024-03-14 14:59 ` Sudeep Holla
2024-03-14 15:16 ` Abdellatif El Khlifi
2024-03-14 16:29 ` Mathieu Poirier
0 siblings, 2 replies; 59+ messages in thread
From: Sudeep Holla @ 2024-03-14 14:59 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Abdellatif El Khlifi, Bjorn Andersson, Sudeep Holla, Rob Herring,
Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley,
Drew.Reed, Adam.Johnston, linux-arm-kernel, devicetree,
linux-kernel, linux-remoteproc
On Thu, Mar 14, 2024 at 08:52:59AM -0600, Mathieu Poirier wrote:
> On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> > Hi Mathieu,
> >
> > On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > > Hi Mathieu,
> > > >
> > > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > (emulator).
> > > > > >
> > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > scenario that needs to be supported and not just a temporary stage.
> > > >
> > > > The current status of the Corstone-1000 SoC requires that there is
> > > > a preloaded firmware for the external core. Preloading is done externally
> > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > on the SoC.
> > > >
> > >
> > > Ok
> > >
> > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > to share memory with the remote core and also being able to access the remote
> > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > of adding remoteproc support for Corstone.
> > > >
> > >
> > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > memory, preventing the A core from copying the firmware image to the proper
> > > location.
> > >
> > > When the HW is fixed, will there be a need to support scenarios where the
> > > firmware image has been preloaded into memory?
> >
> > No, this scenario won't apply when we get the HW upgrade. No need for an
> > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> >
>
> Very well. I am willing to continue with this driver but it does so little that
> I wonder if it wouldn't simply be better to move forward with upstreaming when
> the HW is fixed. The choice is yours.
>
I think Robin has raised few points that need clarification. I think it was
done as part of DT binding patch. I share those concerns and I wanted to
reaching to the same concerns by starting the questions I asked on corstone
device tree changes.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-14 14:59 ` Sudeep Holla
@ 2024-03-14 15:16 ` Abdellatif El Khlifi
2024-03-14 15:24 ` Sudeep Holla
2024-03-14 16:29 ` Mathieu Poirier
1 sibling, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-14 15:16 UTC (permalink / raw)
To: Sudeep Holla
Cc: Robin Murphy, Bjorn Andersson, Mathieu Poirier, Rob Herring,
Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley,
Drew.Reed, Adam.Johnston, linux-arm-kernel, devicetree,
linux-kernel, linux-remoteproc
Hi Sudeep,
On Thu, Mar 14, 2024 at 02:59:20PM +0000, Sudeep Holla wrote:
> On Thu, Mar 14, 2024 at 08:52:59AM -0600, Mathieu Poirier wrote:
> > On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> > > Hi Mathieu,
> > >
> > > On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > > > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > > > Hi Mathieu,
> > > > >
> > > > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > (emulator).
> > > > > > >
> > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > >
> > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > on the SoC.
> > > > >
> > > >
> > > > Ok
> > > >
> > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > to share memory with the remote core and also being able to access the remote
> > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > of adding remoteproc support for Corstone.
> > > > >
> > > >
> > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > memory, preventing the A core from copying the firmware image to the proper
> > > > location.
> > > >
> > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > firmware image has been preloaded into memory?
> > >
> > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > >
> >
> > Very well. I am willing to continue with this driver but it does so little that
> > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > the HW is fixed. The choice is yours.
> >
>
> I think Robin has raised few points that need clarification. I think it was
> done as part of DT binding patch. I share those concerns and I wanted to
> reaching to the same concerns by starting the questions I asked on corstone
> device tree changes.
Please have a look at my answer to Robin with clarifications [1].
Apart from mapping the register area rather than using the reg property
I'll also add the mboxes property as Krzysztof confirmed.
[1]: https://lore.kernel.org/all/20240314134928.GA27077@e130802.arm.com/
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-14 13:49 ` Abdellatif El Khlifi
2024-03-14 13:56 ` Krzysztof Kozlowski
@ 2024-03-14 15:19 ` Sudeep Holla
2024-03-15 14:22 ` Abdellatif El Khlifi
1 sibling, 1 reply; 59+ messages in thread
From: Sudeep Holla @ 2024-03-14 15:19 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: Robin Murphy, Bjorn Andersson, Sudeep Holla, Mathieu Poirier,
Rob Herring, Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
On Thu, Mar 14, 2024 at 01:49:28PM +0000, Abdellatif El Khlifi wrote:
> Hi Robin,
>
> > > + firmware-name:
> > > + description: |
> > > + Default name of the firmware to load to the remote processor.
> >
> > So... is loading the firmware image achieved by somehow bitbanging it
> > through the one reset register, maybe? I find it hard to believe this is a
> > complete and functional binding.
> >
> > Frankly at the moment I'd be inclined to say it isn't even a remoteproc
> > binding (or driver) at all, it's a reset controller. Bindings are a contract
> > for describing the hardware, not the current state of Linux driver support -
> > if this thing still needs mailboxes, shared memory, a reset vector register,
> > or whatever else to actually be useful, those should be in the binding from
> > day 1 so that a) people can write and deploy correct DTs now, such that
> > functionality becomes available on their systems as soon as driver support
> > catches up, and b) the community has any hope of being able to review
> > whether the binding is appropriately designed and specified for the purpose
> > it intends to serve.
>
> This is an initial patchset for allowing to turn on and off the remote processor.
> The FW is already loaded before the Corstone-1000 SoC is powered on and this
> is done through the FPGA board bootloader in case of the FPGA target.
> Or by the Corstone-1000 FVP model (emulator).
>
If this driver does the loading of the firmware, it will get reloaded. Do
you need any issues there ? The correctness matters here in the upstream
driver, it may not be optimised for you usecase now, but that is fine IMO.
> The plan for the driver is as follows:
>
> Step 1: provide a foundation driver capable of turning the core on/off
> Step 2: provide mailbox support for comms
> Step 3: provide FW reload capability
>
> Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can
> share memory with the remote core.
>
Honestly, I would prefer to know the overall design before pushing any partial
solution. If you know the final complete solution, present the same with
the complete device tree binding for better understanding and review.
> So, when memory sharing becomes available in the FPGA and FVP the
> DT binding will be upgraded with:
>
> - mboxes property specifying the RX/TX mailboxes (based on MHU v2)
> - memory-region property describing the virtio vrings
>
Looks like you have the information now, please define the complete
bindings now.
> Currently the mailbox controller does exist in the HW but is not
> usable via virtio (no memory sharing available).
>
That is fine, if the plan is to use them, then include them in the design
of your DT bindings.
> Do you recommend I add the mboxes property even currently we can't do the comms ?
>
Yes for sure IMO.
> > For instance right now it seems somewhat tenuous to describe two consecutive
> > 32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
> > all there ever is. However if it's actually going to end up needing several
> > more additional MMIO and/or memory regions for other functionality, then
> > describing each register and location individually is liable to get
> > unmanageable really fast, and a higher-level functional grouping (e.g. these
> > reset-related registers together as a single 8-byte region) would likely be
> > a better design.
>
I completely agree with the above and this is what I was meant(I must admit
didn't put it forward with above clarity).
> Currently the HW provides only 2 registers to control the remote processors:
>
> The reset control and status registers.
>
Agreed, but it is part of a bigger block with other functionality in place.
MFD/syscon might be better way to use these registers. You never know in
future you might want to use another set of 2-4 registers with a different
functionality in another driver.
> It makes sense to me to use a mapped region of 8 bytes for both registers rather
> than individual registers (since they are consecutive).
Not exactly. Are you sure, Linux will not have to use another other registers
in that block ? Will you keep creating such (random if I may call it so)
bindings for a smaller sets of register under "Host Base System Control
registers".
I would see if it makes sense to put together a single binding for
this "Host Base System Control" register(not sure what exactly that means).
Use MFD/regmap you access parts of this block. The remoteproc driver can
then be semi-generic(meaning applicable to group of similar platforms)
based on the platform compatible and use this regmap to provide the
functionality needed.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-14 13:56 ` Krzysztof Kozlowski
@ 2024-03-14 15:20 ` Abdellatif El Khlifi
0 siblings, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-14 15:20 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Robin Murphy, Bjorn Andersson, Mathieu Poirier, Rob Herring,
Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
Hi Krzysztof,
On Thu, Mar 14, 2024 at 02:56:53PM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2024 14:49, Abdellatif El Khlifi wrote:
> >> Frankly at the moment I'd be inclined to say it isn't even a remoteproc
> >> binding (or driver) at all, it's a reset controller. Bindings are a contract
> >> for describing the hardware, not the current state of Linux driver support -
> >> if this thing still needs mailboxes, shared memory, a reset vector register,
> >> or whatever else to actually be useful, those should be in the binding from
> >> day 1 so that a) people can write and deploy correct DTs now, such that
> >> functionality becomes available on their systems as soon as driver support
> >> catches up, and b) the community has any hope of being able to review
> >> whether the binding is appropriately designed and specified for the purpose
> >> it intends to serve.
> >
> > This is an initial patchset for allowing to turn on and off the remote processor.
> > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > is done through the FPGA board bootloader in case of the FPGA target.
> > Or by the Corstone-1000 FVP model (emulator).
> >
> > The plan for the driver is as follows:
> >
> > Step 1: provide a foundation driver capable of turning the core on/off
> > Step 2: provide mailbox support for comms
> > Step 3: provide FW reload capability
> >
> > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can
> > share memory with the remote core.
> >
> > So, when memory sharing becomes available in the FPGA and FVP the
> > DT binding will be upgraded with:
> >
> > - mboxes property specifying the RX/TX mailboxes (based on MHU v2)
> > - memory-region property describing the virtio vrings
> >
> > Currently the mailbox controller does exist in the HW but is not
> > usable via virtio (no memory sharing available).
> >
> > Do you recommend I add the mboxes property even currently we can't do the comms ?
>
> Bindings should be complete, regardless whether Linux driver supports it
> or not. Please see writing bindings document for explanation on this and
> other rules.
>
> So yes: please describe as much as possible/reasonable.
I'll do thanks.
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-14 15:16 ` Abdellatif El Khlifi
@ 2024-03-14 15:24 ` Sudeep Holla
0 siblings, 0 replies; 59+ messages in thread
From: Sudeep Holla @ 2024-03-14 15:24 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: Robin Murphy, Bjorn Andersson, Sudeep Holla, Mathieu Poirier,
Rob Herring, Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
On Thu, Mar 14, 2024 at 03:16:53PM +0000, Abdellatif El Khlifi wrote:
> Hi Sudeep,
>
> On Thu, Mar 14, 2024 at 02:59:20PM +0000, Sudeep Holla wrote:
> >
> > I think Robin has raised few points that need clarification. I think it was
> > done as part of DT binding patch. I share those concerns and I wanted to
> > reaching to the same concerns by starting the questions I asked on corstone
> > device tree changes.
>
> Please have a look at my answer to Robin with clarifications [1].
>
Yes I did take a look at the response but am not convinced yet. I have
responded to you email with other details which I want to be explored.
I don't think we need to rush to add the *foundation driver* as you call
before the bindings are defined well.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-14 14:59 ` Sudeep Holla
2024-03-14 15:16 ` Abdellatif El Khlifi
@ 2024-03-14 16:29 ` Mathieu Poirier
2024-03-25 17:13 ` Abdellatif El Khlifi
1 sibling, 1 reply; 59+ messages in thread
From: Mathieu Poirier @ 2024-03-14 16:29 UTC (permalink / raw)
To: Sudeep Holla
Cc: Abdellatif El Khlifi, Bjorn Andersson, Rob Herring, Liviu Dudau,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
On Thu, 14 Mar 2024 at 08:59, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Mar 14, 2024 at 08:52:59AM -0600, Mathieu Poirier wrote:
> > On Wed, Mar 13, 2024 at 05:17:56PM +0000, Abdellatif El Khlifi wrote:
> > > Hi Mathieu,
> > >
> > > On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote:
> > > > On Tue, Mar 12, 2024 at 05:32:52PM +0000, Abdellatif El Khlifi wrote:
> > > > > Hi Mathieu,
> > > > >
> > > > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote:
> > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > (emulator).
> > > > > > >
> > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > >
> > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > on the SoC.
> > > > >
> > > >
> > > > Ok
> > > >
> > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > to share memory with the remote core and also being able to access the remote
> > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > of adding remoteproc support for Corstone.
> > > > >
> > > >
> > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > memory, preventing the A core from copying the firmware image to the proper
> > > > location.
> > > >
> > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > firmware image has been preloaded into memory?
> > >
> > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > >
> >
> > Very well. I am willing to continue with this driver but it does so little that
> > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > the HW is fixed. The choice is yours.
> >
>
> I think Robin has raised few points that need clarification. I think it was
> done as part of DT binding patch. I share those concerns and I wanted to
> reaching to the same concerns by starting the questions I asked on corstone
> device tree changes.
>
I also agree with Robin's point of view. Proceeding with an initial
driver with minimal functionality doesn't preclude having complete
bindings. But that said and as I pointed out, it might be better to
wait for the HW to be fixed before moving forward.
> --
> Regards,
> Sudeep
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
2024-03-14 15:19 ` Sudeep Holla
@ 2024-03-15 14:22 ` Abdellatif El Khlifi
0 siblings, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-15 14:22 UTC (permalink / raw)
To: Sudeep Holla
Cc: Robin Murphy, Bjorn Andersson, Sudeep Holla, Mathieu Poirier,
Rob Herring, Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski,
Conor Dooley, Drew.Reed, Adam.Johnston, linux-arm-kernel,
devicetree, linux-kernel, linux-remoteproc
Hi Sudeep,
On Thu, Mar 14, 2024 at 03:19:13PM +0000, Sudeep Holla wrote:
> > The plan for the driver is as follows:
> >
> > Step 1: provide a foundation driver capable of turning the core on/off
> > Step 2: provide mailbox support for comms
> > Step 3: provide FW reload capability
> >
> > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can
> > share memory with the remote core.
> >
>
> Honestly, I would prefer to know the overall design before pushing any partial
> solution. If you know the final complete solution, present the same with
> the complete device tree binding for better understanding and review.
Sounds good to me. I'll make the binding as complete as possible.
> Agreed, but it is part of a bigger block with other functionality in place.
> MFD/syscon might be better way to use these registers. You never know in
> future you might want to use another set of 2-4 registers with a different
> functionality in another driver.
>
> > It makes sense to me to use a mapped region of 8 bytes for both registers rather
> > than individual registers (since they are consecutive).
>
> Not exactly. Are you sure, Linux will not have to use another other registers
> in that block ? Will you keep creating such (random if I may call it so)
> bindings for a smaller sets of register under "Host Base System Control
> registers".
>
> I would see if it makes sense to put together a single binding for
> this "Host Base System Control" register(not sure what exactly that means).
> Use MFD/regmap you access parts of this block. The remoteproc driver can
> then be semi-generic(meaning applicable to group of similar platforms)
> based on the platform compatible and use this regmap to provide the
> functionality needed.
I like the idea of using syscon/regmap to represent the "Host Base System Control registers"
area. Thank you for suggesting that.
I think syscon is the way to go (rather than MFD). With syscon we can use
the generic syscon driver that converts a set of MMIO registers to a regmap,
allowing it to be accessed from multiple device drivers.
In our case these MMIO registers will be the "Host Base System Control registers".
remoteproc will be a child node under sysctrl node.
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-14 16:29 ` Mathieu Poirier
@ 2024-03-25 17:13 ` Abdellatif El Khlifi
2024-03-26 14:20 ` Mathieu Poirier
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-25 17:13 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Sudeep Holla, Abdellatif El Khlifi, Bjorn Andersson, Rob Herring,
Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley,
Drew.Reed, Adam.Johnston, linux-arm-kernel, devicetree,
linux-kernel, linux-remoteproc
Hi Mathieu,
> > > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > > (emulator).
> > > > > > > >
> > > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > > >
> > > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > > on the SoC.
> > > > > >
> > > > >
> > > > > Ok
> > > > >
> > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > > to share memory with the remote core and also being able to access the remote
> > > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > > of adding remoteproc support for Corstone.
> > > > > >
> > > > >
> > > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > > memory, preventing the A core from copying the firmware image to the proper
> > > > > location.
> > > > >
> > > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > > firmware image has been preloaded into memory?
> > > >
> > > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > > >
> > >
> > > Very well. I am willing to continue with this driver but it does so little that
> > > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > > the HW is fixed. The choice is yours.
> > >
> >
> > I think Robin has raised few points that need clarification. I think it was
> > done as part of DT binding patch. I share those concerns and I wanted to
> > reaching to the same concerns by starting the questions I asked on corstone
> > device tree changes.
> >
>
> I also agree with Robin's point of view. Proceeding with an initial
> driver with minimal functionality doesn't preclude having complete
> bindings. But that said and as I pointed out, it might be better to
> wait for the HW to be fixed before moving forward.
We checked with the HW teams. The missing features will be implemented but
this will take time.
The foundation driver as it is right now is still valuable for people wanting to
know how to power control Corstone external systems in a future proof manner
(even in the incomplete state). We prefer to address all the review comments
made so it can be merged. This includes making the DT binding as complete as
possible as you advised. Then, once the HW is ready, I'll implement the comms
and the FW reload part. Is that OK please ?
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-25 17:13 ` Abdellatif El Khlifi
@ 2024-03-26 14:20 ` Mathieu Poirier
2024-03-26 17:14 ` Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
0 siblings, 2 replies; 59+ messages in thread
From: Mathieu Poirier @ 2024-03-26 14:20 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: Sudeep Holla, Bjorn Andersson, Rob Herring, Liviu Dudau,
Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley, Drew.Reed,
Adam.Johnston, linux-arm-kernel, devicetree, linux-kernel,
linux-remoteproc
On Mon, 25 Mar 2024 at 11:13, Abdellatif El Khlifi
<abdellatif.elkhlifi@arm.com> wrote:
>
> Hi Mathieu,
>
> > > > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > > > (emulator).
> > > > > > > > >
> > > > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > > > >
> > > > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > > > on the SoC.
> > > > > > >
> > > > > >
> > > > > > Ok
> > > > > >
> > > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > > > to share memory with the remote core and also being able to access the remote
> > > > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > > > of adding remoteproc support for Corstone.
> > > > > > >
> > > > > >
> > > > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > > > memory, preventing the A core from copying the firmware image to the proper
> > > > > > location.
> > > > > >
> > > > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > > > firmware image has been preloaded into memory?
> > > > >
> > > > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > > > >
> > > >
> > > > Very well. I am willing to continue with this driver but it does so little that
> > > > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > > > the HW is fixed. The choice is yours.
> > > >
> > >
> > > I think Robin has raised few points that need clarification. I think it was
> > > done as part of DT binding patch. I share those concerns and I wanted to
> > > reaching to the same concerns by starting the questions I asked on corstone
> > > device tree changes.
> > >
> >
> > I also agree with Robin's point of view. Proceeding with an initial
> > driver with minimal functionality doesn't preclude having complete
> > bindings. But that said and as I pointed out, it might be better to
> > wait for the HW to be fixed before moving forward.
>
> We checked with the HW teams. The missing features will be implemented but
> this will take time.
>
> The foundation driver as it is right now is still valuable for people wanting to
> know how to power control Corstone external systems in a future proof manner
> (even in the incomplete state). We prefer to address all the review comments
> made so it can be merged. This includes making the DT binding as complete as
> possible as you advised. Then, once the HW is ready, I'll implement the comms
> and the FW reload part. Is that OK please ?
>
I'm in agreement with that plan as long as we agree the current
preloaded heuristic is temporary and is not a valid long term
scenario.
> Cheers,
> Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
2024-03-26 14:20 ` Mathieu Poirier
@ 2024-03-26 17:14 ` Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
1 sibling, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-03-26 17:14 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Abdellatif El Khlifi, Sudeep Holla, Bjorn Andersson, Rob Herring,
Liviu Dudau, Lorenzo Pieralisi, Krzysztof Kozlowski, Conor Dooley,
Drew.Reed, Adam.Johnston, linux-arm-kernel, devicetree,
linux-kernel, linux-remoteproc
Hi Mathieu,
> > > > > > > > > > This is an initial patchset for allowing to turn on and off the remote processor.
> > > > > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on and this
> > > > > > > > > > is done through the FPGA board bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
> > > > > > > > > > (emulator).
> > > > > > > > > >
> > > > > > > > > >From the above I take it that booting with a preloaded firmware is a
> > > > > > > > > scenario that needs to be supported and not just a temporary stage.
> > > > > > > >
> > > > > > > > The current status of the Corstone-1000 SoC requires that there is
> > > > > > > > a preloaded firmware for the external core. Preloading is done externally
> > > > > > > > either through the FPGA bootloader or the emulator (FVP) before powering
> > > > > > > > on the SoC.
> > > > > > > >
> > > > > > >
> > > > > > > Ok
> > > > > > >
> > > > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux is able
> > > > > > > > to share memory with the remote core and also being able to access the remote
> > > > > > > > core memory so Linux can copy the firmware to. This HW changes are still
> > > > > > > > This is why this patchset is relying on a preloaded firmware. And it's the step 1
> > > > > > > > of adding remoteproc support for Corstone.
> > > > > > > >
> > > > > > >
> > > > > > > Ok, so there is a HW problem where A core and M core can't see each other's
> > > > > > > memory, preventing the A core from copying the firmware image to the proper
> > > > > > > location.
> > > > > > >
> > > > > > > When the HW is fixed, will there be a need to support scenarios where the
> > > > > > > firmware image has been preloaded into memory?
> > > > > >
> > > > > > No, this scenario won't apply when we get the HW upgrade. No need for an
> > > > > > external entity anymore. The firmware(s) will all be files in the linux filesystem.
> > > > > >
> > > > >
> > > > > Very well. I am willing to continue with this driver but it does so little that
> > > > > I wonder if it wouldn't simply be better to move forward with upstreaming when
> > > > > the HW is fixed. The choice is yours.
> > > > >
> > > >
> > > > I think Robin has raised few points that need clarification. I think it was
> > > > done as part of DT binding patch. I share those concerns and I wanted to
> > > > reaching to the same concerns by starting the questions I asked on corstone
> > > > device tree changes.
> > > >
> > >
> > > I also agree with Robin's point of view. Proceeding with an initial
> > > driver with minimal functionality doesn't preclude having complete
> > > bindings. But that said and as I pointed out, it might be better to
> > > wait for the HW to be fixed before moving forward.
> >
> > We checked with the HW teams. The missing features will be implemented but
> > this will take time.
> >
> > The foundation driver as it is right now is still valuable for people wanting to
> > know how to power control Corstone external systems in a future proof manner
> > (even in the incomplete state). We prefer to address all the review comments
> > made so it can be merged. This includes making the DT binding as complete as
> > possible as you advised. Then, once the HW is ready, I'll implement the comms
> > and the FW reload part. Is that OK please ?
> >
>
> I'm in agreement with that plan as long as we agree the current
> preloaded heuristic is temporary and is not a valid long term
> scenario.
Yes, that's the plan, no problem.
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems
2024-03-26 14:20 ` Mathieu Poirier
2024-03-26 17:14 ` Abdellatif El Khlifi
@ 2024-08-22 17:09 ` Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors Abdellatif El Khlifi
` (4 more replies)
1 sibling, 5 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-08-22 17:09 UTC (permalink / raw)
To: mathieu.poirier
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, abdellatif.elkhlifi,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
The Corstone-1000 IoT Reference Design Platform [A] supports up to two External
Systems processors.
This patchset allows to control these processors through the remoteproc
subsystem.
The Corstone-1000 implements the SSE-710 subsystem [B] which defines the
MMIO-mapped reset registers (EXT_SYS*) used for controlling the
External Systems [C].
This patchset provides the following:
- Device tree bindings for the SSE-710 subsystem with syscon support
- Device tree bindings for the SSE-710 External System
- Corstone-1000 External System, syscon and MHUs device tree nodes
- Arm MHUv2 Mailbox [F] device tree nodes for Corstone-1000
- Corstone-1000 remoteproc driver with regmap support
For more details, please see the SSE-710 External System Remote
Processor bindings [D] and the SSE-710 Host Base System Control bindings [E].
[A]: https://developer.arm.com/Processors/Corstone-1000
[B]: https://developer.arm.com/documentation/102342/latest/
[C]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
[D]: Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
[E]: Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
[F]: Documentation/devicetree/bindings/mailbox/arm,mhuv2.yaml
Changelog:
============
v2:
* provide SSE-710 syscon bindings
* provide SSE-710 External System bindings
* add Corstone-1000 External System node under syscon
* add Arm MHUv2 Mailbox device tree nodes for Corstone-1000
* add regmap support for the driver
* use devm_rproc_* APIs
* refactoring
v1: [1]
* introduce the Corstone-1000 remoteproc support
List of previous patches:
[1]: https://lore.kernel.org/all/20240301164227.339208-1-abdellatif.elkhlifi@arm.com/
Cheers,
Abdellatif
Abdellatif El Khlifi (5):
dt-bindings: remoteproc: sse710: Add the External Systems remote
processors
dt-bindings: arm: sse710: Add Host Base System Control
arm64: dts: corstone1000: Add MHU nodes used by the External System
arm64: dts: corstone1000: Add External System support
remoteproc: arm64: corstone1000: Add the External Systems driver
.../arm/arm,sse710-host-base-sysctrl.yaml | 56 +++
.../remoteproc/arm,sse710-extsys.yaml | 90 +++++
arch/arm64/boot/dts/arm/corstone1000.dtsi | 34 +-
drivers/remoteproc/Kconfig | 14 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/corstone1000_rproc.c | 350 ++++++++++++++++++
6 files changed, 544 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
create mode 100644 drivers/remoteproc/corstone1000_rproc.c
base-commit: 8fa052c29e509f3e47d56d7fc2ca28094d78c60a
--
2.25.1
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
@ 2024-08-22 17:09 ` Abdellatif El Khlifi
2024-08-22 18:25 ` Rob Herring (Arm)
` (2 more replies)
2024-08-22 17:09 ` [PATCH v2 2/5] dt-bindings: arm: sse710: Add Host Base System Control Abdellatif El Khlifi
` (3 subsequent siblings)
4 siblings, 3 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-08-22 17:09 UTC (permalink / raw)
To: mathieu.poirier
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, abdellatif.elkhlifi,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Add devicetree binding schema for the External Systems remote processors
The External Systems remote processors are provided on the Corstone-1000
IoT Reference Design Platform via the SSE-710 subsystem.
For more details about the External Systems, please see Corstone SSE-710
subsystem features [1].
[1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
.../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
1 file changed, 90 insertions(+)
create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
new file mode 100644
index 000000000000..827ba8d962f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SSE-710 External System Remote Processor
+
+maintainers:
+ - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
+ - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
+
+description: |
+ SSE-710 is an heterogeneous subsystem supporting up to two remote
+ processors aka the External Systems.
+
+properties:
+ compatible:
+ enum:
+ - arm,sse710-extsys
+
+ firmware-name:
+ description:
+ The default name of the firmware to load to the remote processor.
+
+ '#extsys-id':
+ description:
+ The External System ID.
+ enum: [0, 1]
+
+ mbox-names:
+ items:
+ - const: txes0
+ - const: rxes0
+
+ mboxes:
+ description:
+ The list of Message Handling Unit (MHU) channels used for bidirectional
+ communication. This property is only required if the virtio-based Rpmsg
+ messaging bus is used. For more details see the Arm MHUv2 Mailbox
+ Controller at devicetree/bindings/mailbox/arm,mhuv2.yaml
+
+ minItems: 2
+ maxItems: 2
+
+ memory-region:
+ description:
+ If present, a phandle for a reserved memory area that used for vdev
+ buffer, resource table, vring region and others used by the remote
+ processor.
+ minItems: 2
+ maxItems: 32
+
+required:
+ - compatible
+ - firmware-name
+ - '#extsys-id'
+
+additionalProperties: false
+
+examples:
+ - |
+ reserved-memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ extsys0_vring0: vdev0vring0@82001000 {
+ reg = <0 0x82001000 0 0x8000>;
+ no-map;
+ };
+
+ extsys0_vring1: vdev0vring1@82009000 {
+ reg = <0 0x82009000 0 0x8000>;
+ no-map;
+ };
+ };
+
+ syscon@1a010000 {
+ compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
+ reg = <0x1a010000 0x1000>;
+
+ extsys0 {
+ compatible = "arm,sse710-extsys";
+ #extsys-id = <0>;
+ firmware-name = "es_flashfw.elf";
+ mbox-names = "txes0", "rxes0";
+ mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
+ memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
+ };
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 2/5] dt-bindings: arm: sse710: Add Host Base System Control
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors Abdellatif El Khlifi
@ 2024-08-22 17:09 ` Abdellatif El Khlifi
2024-08-23 6:25 ` Krzysztof Kozlowski
2024-08-22 17:09 ` [PATCH v2 3/5] arm64: dts: corstone1000: Add MHU nodes used by the External System Abdellatif El Khlifi
` (2 subsequent siblings)
4 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-08-22 17:09 UTC (permalink / raw)
To: mathieu.poirier
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, abdellatif.elkhlifi,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Add devicetree binding schema for the SSE-710 Host Base System Control
SSE-710 is implemented by the Corstone-1000 IoT Reference Design
Platform [1].
The Host Base System Control has registers to control the clocks, power,
and reset for SSE-710 subsystem [2]. It resides within AONTOP power domain.
The registers are mapped under the SSE-710 Host System memory map [3].
[1]: https://developer.arm.com/Processors/Corstone-1000
[2]: https://developer.arm.com/documentation/102342/latest/
[3]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
.../arm/arm,sse710-host-base-sysctrl.yaml | 56 +++++++++++++++++++
1 file changed, 56 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
diff --git a/Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml b/Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
new file mode 100644
index 000000000000..e344a73e329d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/arm,sse710-host-base-sysctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SSE-710 Host Base System Control
+
+maintainers:
+ - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
+ - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
+
+description: |+
+ The Host Base System Control has registers to control the clocks, power, and
+ reset for SSE-710 subsystem. It resides within AONTOP power domain.
+ The registers are mapped under the SSE-710 Host System memory map.
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - arm,sse710-host-base-sysctrl
+ - const: simple-mfd
+ - const: syscon
+
+ reg:
+ maxItems: 1
+
+patternProperties:
+ "^extsys[0-1]$":
+ description:
+ SSE-710 subsystem supports up to two External Systems.
+ $ref: /schemas/remoteproc/arm,sse710-extsys.yaml#
+ unevaluatedProperties: false
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ syscon@1a010000 {
+ compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
+ reg = <0x1a010000 0x1000>;
+
+ extsys0 {
+ compatible = "arm,sse710-extsys";
+ firmware-name = "es_flashfw.elf";
+ #extsys-id = <0>;
+ mbox-names = "txes0", "rxes0";
+ mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
+ memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
+ };
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 3/5] arm64: dts: corstone1000: Add MHU nodes used by the External System
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 2/5] dt-bindings: arm: sse710: Add Host Base System Control Abdellatif El Khlifi
@ 2024-08-22 17:09 ` Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 4/5] arm64: dts: corstone1000: Add External System support Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 5/5] remoteproc: arm64: corstone1000: Add the External Systems driver Abdellatif El Khlifi
4 siblings, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-08-22 17:09 UTC (permalink / raw)
To: mathieu.poirier
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, abdellatif.elkhlifi,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Add normal world mhu0_hes0 and mhu0_es0h nodes
In Corstone-1000 IoT Reference Design Platform, communication between the
host (Cortex-A35) running in normal world (EL0 and EL1) and the external
system (Cortex-M3) is done with MHU0.
MHU0 is a bidirectional communication channel described in the device tree
through mhu0_hes0 and mhu0_es0h.
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
arch/arm64/boot/dts/arm/corstone1000.dtsi | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/arm/corstone1000.dtsi b/arch/arm64/boot/dts/arm/corstone1000.dtsi
index bb9b96fb5314..01c65195ca53 100644
--- a/arch/arm64/boot/dts/arm/corstone1000.dtsi
+++ b/arch/arm64/boot/dts/arm/corstone1000.dtsi
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0 OR MIT
/*
- * Copyright (c) 2022, Arm Limited. All rights reserved.
+ * Copyright (c) 2022, 2024 Arm Limited. All rights reserved.
* Copyright (c) 2022, Linaro Limited. All rights reserved.
*
*/
@@ -134,6 +134,26 @@ uart1: serial@1a520000 {
clock-names = "uartclk", "apb_pclk";
};
+ mhu0_hes0: mhu@1b000000 {
+ compatible = "arm,mhuv2-tx","arm,primecell";
+ reg = <0x1b000000 0x1000>;
+ clocks = <&refclk100mhz>;
+ clock-names = "apb_pclk";
+ interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+ #mbox-cells = <2>;
+ arm,mhuv2-protocols = <0 1>;
+ };
+
+ mhu0_es0h: mhu@1b010000 {
+ compatible = "arm,mhuv2-rx","arm,primecell";
+ reg = <0x1b010000 0x1000>;
+ clocks = <&refclk100mhz>;
+ clock-names = "apb_pclk";
+ interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
+ #mbox-cells = <2>;
+ arm,mhuv2-protocols = <0 1>;
+ };
+
mhu_hse1: mailbox@1b820000 {
compatible = "arm,mhuv2-tx", "arm,primecell";
reg = <0x1b820000 0x1000>;
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 4/5] arm64: dts: corstone1000: Add External System support
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
` (2 preceding siblings ...)
2024-08-22 17:09 ` [PATCH v2 3/5] arm64: dts: corstone1000: Add MHU nodes used by the External System Abdellatif El Khlifi
@ 2024-08-22 17:09 ` Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 5/5] remoteproc: arm64: corstone1000: Add the External Systems driver Abdellatif El Khlifi
4 siblings, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-08-22 17:09 UTC (permalink / raw)
To: mathieu.poirier
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, abdellatif.elkhlifi,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Add extsys0 remoteproc node as a child node of syscon
extsys0 describes the Corstone-1000 external system [1]
(the remote processor).
The host (Cortex-A35) can control the external system through memory mapped
registers located in a memory area called the
Host Base System Control [2][3]. This area is part of the host memory
space.
We use syscon to represent the Host Base System Control area and the
remoteproc node is a child node.
[1]: Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
[2]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
[3]: Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
arch/arm64/boot/dts/arm/corstone1000.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/arm/corstone1000.dtsi b/arch/arm64/boot/dts/arm/corstone1000.dtsi
index 01c65195ca53..17d6638f9ca6 100644
--- a/arch/arm64/boot/dts/arm/corstone1000.dtsi
+++ b/arch/arm64/boot/dts/arm/corstone1000.dtsi
@@ -103,6 +103,18 @@ soc {
interrupt-parent = <&gic>;
ranges;
+ syscon@1a010000 {
+ compatible = "arm,sse710-host-base-sysctrl",
+ "simple-mfd", "syscon";
+ reg = <0x1a010000 0x1000>;
+
+ extsys0 {
+ compatible = "arm,sse710-extsys";
+ #extsys-id = <0>;
+ firmware-name = "es_flashfw.elf";
+ };
+ };
+
timer@1a220000 {
compatible = "arm,armv7-timer-mem";
reg = <0x1a220000 0x1000>;
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH v2 5/5] remoteproc: arm64: corstone1000: Add the External Systems driver
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
` (3 preceding siblings ...)
2024-08-22 17:09 ` [PATCH v2 4/5] arm64: dts: corstone1000: Add External System support Abdellatif El Khlifi
@ 2024-08-22 17:09 ` Abdellatif El Khlifi
2024-09-18 15:40 ` Abdellatif El Khlifi
4 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-08-22 17:09 UTC (permalink / raw)
To: mathieu.poirier
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, abdellatif.elkhlifi,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Introduce remoteproc support for Corstone-1000 external systems
The Corstone-1000 IoT Reference Design Platform supports up to two
external systems processors. These processors can be switched on or off
using their reset registers.
For more details, please see the SSE-710 External System Remote
Processor binding [1] and the SSE-710 Host Base System Control binding [2].
The reset registers are MMIO mapped registers accessed using regmap.
[1]: Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
[2]: Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
---
drivers/remoteproc/Kconfig | 14 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/corstone1000_rproc.c | 350 ++++++++++++++++++++++++
3 files changed, 365 insertions(+)
create mode 100644 drivers/remoteproc/corstone1000_rproc.c
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 0f0862e20a93..a0ff5d4f2319 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -379,6 +379,20 @@ config XLNX_R5_REMOTEPROC
It's safe to say N if not interested in using RPU r5f cores.
+config CORSTONE1000_REMOTEPROC
+ tristate "Arm Corstone-1000 remoteproc support"
+ depends on ARM64 || (HAS_IOMEM && COMPILE_TEST)
+ help
+ Say y here to support Arm Corstone-1000 remote processors via the
+ remote processor framework.
+
+ Corstone-1000 remote processors are controlled with a reset status
+ and control registers. The driver also supports control of multiple
+ remote cores at the same time.
+
+ It's safe to say N here if not interested in utilizing remote
+ processors.
+
endif # REMOTEPROC
endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 5ff4e2fee4ab..e017f75143e3 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -40,3 +40,4 @@ obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
obj-$(CONFIG_TI_K3_M4_REMOTEPROC) += ti_k3_m4_remoteproc.o
obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
obj-$(CONFIG_XLNX_R5_REMOTEPROC) += xlnx_r5_remoteproc.o
+obj-$(CONFIG_CORSTONE1000_REMOTEPROC) += corstone1000_rproc.o
diff --git a/drivers/remoteproc/corstone1000_rproc.c b/drivers/remoteproc/corstone1000_rproc.c
new file mode 100644
index 000000000000..bf351af6a1c3
--- /dev/null
+++ b/drivers/remoteproc/corstone1000_rproc.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Arm Corstone-1000 Remoteproc driver
+ *
+ * The driver adds remoteproc support for the external cores used in Arm
+ * Corstone-1000 IoT Reference Design Platform [1][2]
+ * [1] Arm Corstone-1000 Technical Overview: https://developer.arm.com/documentation/102360/0000
+ * [2] Arm Corstone SSE-710 Subsystem Technical Reference Manual: https://developer.arm.com/documentation/102342/0000
+ *
+ * Copyright (C) 2024 ARM Ltd.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+/**
+ * struct corstone1000_rproc - Arm remote processor instance
+ * @rproc: rproc handler
+ * @regmap: MMIO register map
+ * @ctrl_reg: control register offset
+ * @state_reg: state register offset
+ */
+struct corstone1000_rproc {
+ struct rproc *rproc;
+ struct regmap *regmap;
+ u16 ctrl_reg;
+ u16 state_reg;
+};
+
+/* Definitions for Arm Corstone-1000 External System */
+
+/* External Systems identifiers */
+#define EXT_SYS0_ID (0) /* External System 0 ID */
+#define EXT_SYS1_ID (1) /* External System 1 ID */
+
+/* External System 0 registers offset */
+#define EXT_SYS0_RST_CTRL (0x310) /* Reset Control register */
+#define EXT_SYS0_RST_ST (0x314) /* Reset Status register */
+
+/* External System 1 registers offset */
+#define EXT_SYS1_RST_CTRL (0x318) /* Reset Control register */
+#define EXT_SYS1_RST_ST (0x31c) /* Reset Status register */
+
+/* External System Reset Control register bit definitions */
+#define EXTSYS_RST_CTRL_CPUWAIT BIT(0) /* CPU Wait control */
+#define EXTSYS_RST_CTRL_RST_REQ BIT(1) /*Reset request */
+
+/* Status of reset request bits */
+#define EXTSYS_RST_ACK_MASK GENMASK(2, 1)
+#define GET_EXTSYS_RST_ST_RST_ACK(x) ((u8)(FIELD_GET(EXTSYS_RST_ACK_MASK, \
+ (x))))
+
+/* Possible values for the Status of reset request */
+#define EXTSYS_RST_ACK_NO_RESET_REQ (0x0)
+#define EXTSYS_RST_ACK_NOT_COMPLETE (0x1)
+#define EXTSYS_RST_ACK_COMPLETE (0x2)
+#define EXTSYS_RST_ACK_RESERVED (0x3)
+
+/* Polling settings used when reading the Status of reset request */
+#define EXTSYS_RST_ACK_POLL_TRIES (3)
+#define EXTSYS_RST_ACK_POLL_TIMEOUT (1000)
+
+static int corstone1000_rproc_extsys_poll_rst_ack(struct rproc *rproc,
+ u8 exp_ack, u8 *rst_ack);
+
+/**
+ * corstone1000_rproc_start() - custom start operation
+ * @rproc: pointer to the remote processor object
+ *
+ * Start function for Corstone-1000 External System.
+ * Allow the External System core start execute instructions.
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int corstone1000_rproc_start(struct rproc *rproc)
+{
+ struct corstone1000_rproc *priv = rproc->priv;
+
+ /* CPUWAIT signal of the External System is de-asserted */
+
+ return regmap_write_bits(priv->regmap, priv->ctrl_reg,
+ EXTSYS_RST_CTRL_CPUWAIT,
+ (unsigned int)~EXTSYS_RST_CTRL_CPUWAIT);
+}
+
+/**
+ * corstone1000_rproc_stop() - custom stop operation
+ * @rproc: pointer to the remote processor object
+ *
+ * Reset all logic within the External System, the core will be in a halt state.
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int corstone1000_rproc_stop(struct rproc *rproc)
+{
+ struct corstone1000_rproc *priv = rproc->priv;
+ u8 rst_ack, req_status;
+ int ret;
+
+ ret = regmap_write_bits(priv->regmap, priv->ctrl_reg,
+ EXTSYS_RST_CTRL_RST_REQ,
+ EXTSYS_RST_CTRL_RST_REQ);
+ if (ret)
+ return ret;
+
+ ret = corstone1000_rproc_extsys_poll_rst_ack(rproc,
+ EXTSYS_RST_ACK_COMPLETE |
+ EXTSYS_RST_ACK_NOT_COMPLETE,
+ &rst_ack);
+ if (ret)
+ return ret;
+
+ req_status = rst_ack;
+
+ ret = regmap_write_bits(priv->regmap, priv->ctrl_reg,
+ EXTSYS_RST_CTRL_RST_REQ,
+ (unsigned int)~EXTSYS_RST_CTRL_RST_REQ);
+ if (ret)
+ return ret;
+
+ ret = corstone1000_rproc_extsys_poll_rst_ack(rproc,
+ EXTSYS_RST_ACK_NO_RESET_REQ,
+ &rst_ack);
+ if (ret)
+ return ret;
+
+ if (req_status == EXTSYS_RST_ACK_COMPLETE)
+ return 0;
+
+ return -EACCES;
+}
+
+/**
+ * corstone1000_rproc_parse_fw() - Parse firmware operation
+ * @rproc: pointer to the remote processor object
+ * @fw: pointer to the firmware
+ *
+ * Does nothing currently. No resource information needed from the firmware
+ * image.
+ *
+ * Return:
+ *
+ * 0 for success.
+ */
+static int corstone1000_rproc_parse_fw(struct rproc *rproc,
+ const struct firmware *fw)
+{
+ return 0;
+}
+
+/**
+ * corstone1000_rproc_load() - Load firmware to memory operation
+ * @rproc: pointer to the remote processor object
+ * @fw: pointer to the firmware
+ *
+ * The remoteproc subsystem expects a firmware image to be provided and loaded
+ * into memory. In case of Corstone-1000, the firmware is already loaded before
+ * the Corstone-1000 is powered on and this is done through the FPGA board
+ * bootloader in case of the FPGA target. Or by the Corstone-1000 FVP model
+ * when using the FVP target.
+ *
+ * Return:
+ *
+ * 0 for success.
+ */
+static int corstone1000_rproc_load(struct rproc *rproc,
+ const struct firmware *fw)
+{
+ return 0;
+}
+
+static const struct rproc_ops corstone1000_rproc_ops = {
+ .start = corstone1000_rproc_start,
+ .stop = corstone1000_rproc_stop,
+ .load = corstone1000_rproc_load,
+ .parse_fw = corstone1000_rproc_parse_fw,
+};
+
+/**
+ * corstone1000_rproc_extsys_poll_rst_ack() - poll RST_ACK bits
+ * @rproc: pointer to the remote processor object
+ * @exp_ack: expected bits value
+ * @rst_ack: bits value read
+ *
+ * Tries to read RST_ACK bits until the timeout expires.
+ * EXTSYS_RST_ACK_POLL_TRIES tries are made,
+ * every EXTSYS_RST_ACK_POLL_TIMEOUT milliseconds.
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int corstone1000_rproc_extsys_poll_rst_ack(struct rproc *rproc,
+ u8 exp_ack, u8 *rst_ack)
+{
+ struct device *dev;
+ struct corstone1000_rproc *priv;
+ int tries = EXTSYS_RST_ACK_POLL_TRIES;
+ int ret;
+ unsigned long timeout;
+ u32 state_reg;
+
+ if (!rproc || !rst_ack)
+ return -ENODATA;
+
+ dev = rproc->dev.parent;
+ priv = rproc->priv;
+
+ do {
+ ret = regmap_read(priv->regmap, priv->state_reg, &state_reg);
+ if (ret)
+ return ret;
+
+ *rst_ack = GET_EXTSYS_RST_ST_RST_ACK(state_reg);
+
+ if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
+ dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
+ *rst_ack);
+ return -EINVAL;
+ }
+
+ /*
+ * when RST_REQ bit is cleared (No reset requested)
+ * RST_ACK is expected to be 00 (No reset requested)
+ */
+ if (exp_ack == EXTSYS_RST_ACK_NO_RESET_REQ &&
+ *rst_ack == exp_ack)
+ return 0;
+
+ /*
+ * when a reset is requested (RST_REQ set) , RST_ACK is either
+ * 01 (Reset request unable to complete) or 10 (Reset request
+ * complete)
+ */
+ if (*rst_ack & exp_ack)
+ return 0;
+
+ timeout = msleep_interruptible(EXTSYS_RST_ACK_POLL_TIMEOUT);
+
+ if (timeout) {
+ dev_err(dev, "polling RST_ACK aborted\n");
+ return -ECONNABORTED;
+ }
+ } while (--tries);
+
+ dev_err(dev, "polling RST_ACK timed out\n");
+
+ return -ETIMEDOUT;
+}
+
+/**
+ * corstone1000_rproc_probe() - the platform device probe
+ * @pdev: the platform device
+ *
+ * Setup an rproc device and regmap using syscon
+ *
+ * Return:
+ *
+ * 0 on success. Otherwise, failure
+ */
+static int corstone1000_rproc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct device_node *parent;
+ struct corstone1000_rproc *priv;
+ struct rproc *rproc;
+ const char *fw_name;
+ struct regmap *regmap;
+ int ret;
+ u8 extsys_id = EXT_SYS0_ID;
+
+ ret = rproc_of_parse_firmware(dev, 0, &fw_name);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "can't parse firmware-name from DT\n");
+
+ dev_dbg(dev, "firmware-name: %s\n", fw_name);
+
+ rproc = devm_rproc_alloc(dev, np->name, &corstone1000_rproc_ops, fw_name,
+ sizeof(*priv));
+ if (!rproc)
+ return dev_err_probe(dev, -ENOMEM,
+ "can't allocate rproc device\n");
+
+ priv = rproc->priv;
+ priv->rproc = rproc;
+
+ parent = of_get_parent(dev->of_node); /* parent is syscon node */
+ regmap = syscon_node_to_regmap(parent);
+ of_node_put(parent);
+ if (IS_ERR_OR_NULL(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "can't read syscon node\n");
+ priv->regmap = regmap;
+
+ ret = of_property_read_u8(np, "#extsys-id", &extsys_id);
+ if (ret)
+ return dev_err_probe(dev, ret, "can't read '#extsys-id' property\n");
+
+ if (extsys_id == EXT_SYS0_ID) {
+ priv->ctrl_reg = EXT_SYS0_RST_CTRL;
+ priv->state_reg = EXT_SYS0_RST_ST;
+ } else {
+ priv->ctrl_reg = EXT_SYS1_RST_CTRL;
+ priv->state_reg = EXT_SYS1_RST_ST;
+ }
+
+ platform_set_drvdata(pdev, priv);
+
+ ret = devm_rproc_add(dev, rproc);
+ if (ret)
+ return dev_err_probe(dev, ret, "can't add remote processor\n");
+
+ return 0;
+}
+
+static const struct of_device_id corstone1000_rproc_of_match[] = {
+ { .compatible = "arm,sse710-extsys"},
+ { /* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, corstone1000_rproc_of_match);
+
+static struct platform_driver corstone1000_rproc_driver = {
+ .probe = corstone1000_rproc_probe,
+ .driver = {
+ .name = "corstone1000-rproc",
+ .of_match_table = corstone1000_rproc_of_match,
+ },
+};
+module_platform_driver(corstone1000_rproc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Arm Corstone-1000 Remote Processor Control Driver");
+MODULE_AUTHOR("Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>");
--
2.25.1
^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-08-22 17:09 ` [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors Abdellatif El Khlifi
@ 2024-08-22 18:25 ` Rob Herring (Arm)
2024-08-23 6:23 ` Krzysztof Kozlowski
2024-09-27 17:54 ` Robin Murphy
2 siblings, 0 replies; 59+ messages in thread
From: Rob Herring (Arm) @ 2024-08-22 18:25 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: robin.murphy, devicetree, Hugues.KambaMpiana, linux-kernel,
liviu.dudau, Adam.Johnston, mathieu.poirier, sudeep.holla,
Drew.Reed, linux-remoteproc, linux-arm-kernel, conor+dt,
krzysztof.kozlowski+dt, lpieralisi, andersson
On Thu, 22 Aug 2024 18:09:47 +0100, Abdellatif El Khlifi wrote:
> Add devicetree binding schema for the External Systems remote processors
>
> The External Systems remote processors are provided on the Corstone-1000
> IoT Reference Design Platform via the SSE-710 subsystem.
>
> For more details about the External Systems, please see Corstone SSE-710
> subsystem features [1].
>
> [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.example.dtb: /example-0/syscon@1a010000: failed to match any schema with compatible: ['arm,sse710-host-base-sysctrl', 'simple-mfd', 'syscon']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240822170951.339492-2-abdellatif.elkhlifi@arm.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-08-22 17:09 ` [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors Abdellatif El Khlifi
2024-08-22 18:25 ` Rob Herring (Arm)
@ 2024-08-23 6:23 ` Krzysztof Kozlowski
2024-09-19 9:35 ` Abdellatif El Khlifi
2024-09-27 17:54 ` Robin Murphy
2 siblings, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-23 6:23 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On Thu, Aug 22, 2024 at 06:09:47PM +0100, Abdellatif El Khlifi wrote:
> Add devicetree binding schema for the External Systems remote processors
>
> The External Systems remote processors are provided on the Corstone-1000
> IoT Reference Design Platform via the SSE-710 subsystem.
>
> For more details about the External Systems, please see Corstone SSE-710
> subsystem features [1].
>
Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.
> [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> new file mode 100644
> index 000000000000..827ba8d962f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SSE-710 External System Remote Processor
> +
> +maintainers:
> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> + - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
> +
> +description: |
dt-preserve-formatting
> + SSE-710 is an heterogeneous subsystem supporting up to two remote
> + processors aka the External Systems.
> +
> +properties:
> + compatible:
> + enum:
> + - arm,sse710-extsys
> +
> + firmware-name:
> + description:
> + The default name of the firmware to load to the remote processor.
> +
> + '#extsys-id':
'#' is not correct for sure, that's not a cell specifier.
But anyway, we do not accept in general instance IDs.
> + description:
> + The External System ID.
This tells me nothing. You basically copied property name.
> + enum: [0, 1]
> +
> + mbox-names:
> + items:
> + - const: txes0
> + - const: rxes0
> +
> + mboxes:
> + description:
> + The list of Message Handling Unit (MHU) channels used for bidirectional
> + communication. This property is only required if the virtio-based Rpmsg
> + messaging bus is used. For more details see the Arm MHUv2 Mailbox
> + Controller at devicetree/bindings/mailbox/arm,mhuv2.yaml
> +
Drop blank line
> + minItems: 2
This is redundant if equals to maxItemns, drop.
> + maxItems: 2
> +
> + memory-region:
> + description:
> + If present, a phandle for a reserved memory area that used for vdev
> + buffer, resource table, vring region and others used by the remote
> + processor.
> + minItems: 2
> + maxItems: 32
> +
> +required:
> + - compatible
> + - firmware-name
> + - '#extsys-id'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + reserved-memory {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + extsys0_vring0: vdev0vring0@82001000 {
> + reg = <0 0x82001000 0 0x8000>;
> + no-map;
> + };
> +
> + extsys0_vring1: vdev0vring1@82009000 {
> + reg = <0 0x82009000 0 0x8000>;
> + no-map;
> + };
> + };
Drop, it is fairly common.
> +
> + syscon@1a010000 {
> + compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> + reg = <0x1a010000 0x1000>;
So this is a part of other block? Then make one complete example in the
parent device bindings.
> +
> + extsys0 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
e.g. remoteproc
> + compatible = "arm,sse710-extsys";
> + #extsys-id = <0>;
> + firmware-name = "es_flashfw.elf";
> + mbox-names = "txes0", "rxes0";
> + mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
First go mboxes, then mbox-names. The same in the binding, BTW.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: arm: sse710: Add Host Base System Control
2024-08-22 17:09 ` [PATCH v2 2/5] dt-bindings: arm: sse710: Add Host Base System Control Abdellatif El Khlifi
@ 2024-08-23 6:25 ` Krzysztof Kozlowski
0 siblings, 0 replies; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-23 6:25 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On Thu, Aug 22, 2024 at 06:09:48PM +0100, Abdellatif El Khlifi wrote:
> Add devicetree binding schema for the SSE-710 Host Base System Control
>
> SSE-710 is implemented by the Corstone-1000 IoT Reference Design
> Platform [1].
>
> The Host Base System Control has registers to control the clocks, power,
> and reset for SSE-710 subsystem [2]. It resides within AONTOP power domain.
> The registers are mapped under the SSE-710 Host System memory map [3].
>
> [1]: https://developer.arm.com/Processors/Corstone-1000
> [2]: https://developer.arm.com/documentation/102342/latest/
> [3]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> .../arm/arm,sse710-host-base-sysctrl.yaml | 56 +++++++++++++++++++
> 1 file changed, 56 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
>
> diff --git a/Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml b/Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
> new file mode 100644
> index 000000000000..e344a73e329d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/arm,sse710-host-base-sysctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SSE-710 Host Base System Control
> +
> +maintainers:
> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> + - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
> +
> +description: |+
Drop |+
> + The Host Base System Control has registers to control the clocks, power, and
> + reset for SSE-710 subsystem. It resides within AONTOP power domain.
> + The registers are mapped under the SSE-710 Host System memory map.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - arm,sse710-host-base-sysctrl
> + - const: simple-mfd
> + - const: syscon
> +
> + reg:
> + maxItems: 1
> +
> +patternProperties:
> + "^extsys[0-1]$":
^remoteproc-[01]$
> + description:
> + SSE-710 subsystem supports up to two External Systems.
> + $ref: /schemas/remoteproc/arm,sse710-extsys.yaml#
> + unevaluatedProperties: false
> +
> +additionalProperties: false
This goes after "required:" block.
> +
> +required:
> + - compatible
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 5/5] remoteproc: arm64: corstone1000: Add the External Systems driver
2024-08-22 17:09 ` [PATCH v2 5/5] remoteproc: arm64: corstone1000: Add the External Systems driver Abdellatif El Khlifi
@ 2024-09-18 15:40 ` Abdellatif El Khlifi
2024-09-19 8:37 ` Mathieu Poirier
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-09-18 15:40 UTC (permalink / raw)
To: mathieu.poirier; +Cc: linux-arm-kernel, linux-kernel, linux-remoteproc
Hi Mathieu,
> Introduce remoteproc support for Corstone-1000 external systems
>
> The Corstone-1000 IoT Reference Design Platform supports up to two
> external systems processors. These processors can be switched on or off
> using their reset registers.
>
> For more details, please see the SSE-710 External System Remote
> Processor binding [1] and the SSE-710 Host Base System Control binding [2].
>
> The reset registers are MMIO mapped registers accessed using regmap.
>
> [1]: Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> [2]: Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> drivers/remoteproc/Kconfig | 14 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/corstone1000_rproc.c | 350 ++++++++++++++++++++++++
> 3 files changed, 365 insertions(+)
A gentle reminder about reviewing the driver please.
I'll be addressing the comments made for the bindings.
Thank you in advance.
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 5/5] remoteproc: arm64: corstone1000: Add the External Systems driver
2024-09-18 15:40 ` Abdellatif El Khlifi
@ 2024-09-19 8:37 ` Mathieu Poirier
0 siblings, 0 replies; 59+ messages in thread
From: Mathieu Poirier @ 2024-09-19 8:37 UTC (permalink / raw)
To: Abdellatif El Khlifi; +Cc: linux-arm-kernel, linux-kernel, linux-remoteproc
On Wed, 18 Sept 2024 at 09:40, Abdellatif El Khlifi
<abdellatif.elkhlifi@arm.com> wrote:
>
> Hi Mathieu,
>
> > Introduce remoteproc support for Corstone-1000 external systems
> >
> > The Corstone-1000 IoT Reference Design Platform supports up to two
> > external systems processors. These processors can be switched on or off
> > using their reset registers.
> >
> > For more details, please see the SSE-710 External System Remote
> > Processor binding [1] and the SSE-710 Host Base System Control binding [2].
> >
> > The reset registers are MMIO mapped registers accessed using regmap.
> >
> > [1]: Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> > [2]: Documentation/devicetree/bindings/arm/arm,sse710-host-base-sysctrl.yaml
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > ---
> > drivers/remoteproc/Kconfig | 14 +
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/corstone1000_rproc.c | 350 ++++++++++++++++++++++++
> > 3 files changed, 365 insertions(+)
>
> A gentle reminder about reviewing the driver please.
>
> I'll be addressing the comments made for the bindings.
Please address the comments already received for the bindings. I will
review this set once the bindings have settled.
Thanks,
Mathieu
>
> Thank you in advance.
>
> Cheers,
> Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-08-23 6:23 ` Krzysztof Kozlowski
@ 2024-09-19 9:35 ` Abdellatif El Khlifi
2024-09-19 10:04 ` Krzysztof Kozlowski
2024-09-22 18:58 ` Krzysztof Kozlowski
0 siblings, 2 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-09-19 9:35 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Hi Krzysztof,
> > Add devicetree binding schema for the External Systems remote processors
> >
> > The External Systems remote processors are provided on the Corstone-1000
> > IoT Reference Design Platform via the SSE-710 subsystem.
> >
> > For more details about the External Systems, please see Corstone SSE-710
> > subsystem features [1].
> >
>
> Do not attach (thread) your patchsets to some other threads (unrelated
> or older versions). This buries them deep in the mailbox and might
> interfere with applying entire sets.
>
> > [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > ---
> > .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
> > 1 file changed, 90 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> > new file mode 100644
> > index 000000000000..827ba8d962f1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SSE-710 External System Remote Processor
> > +
> > +maintainers:
> > + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > + - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
> > +
> > +description: |
>
> dt-preserve-formatting
Do you mean I should remove the '|' please ? (I didn't find examples of use of
dt-preserve-formatting in Documentation/devicetree/bindings/)
>
> > + SSE-710 is an heterogeneous subsystem supporting up to two remote
> > + processors aka the External Systems.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - arm,sse710-extsys
> > +
> > + firmware-name:
> > + description:
> > + The default name of the firmware to load to the remote processor.
> > +
> > + '#extsys-id':
>
> '#' is not correct for sure, that's not a cell specifier.
>
> But anyway, we do not accept in general instance IDs.
I'm happy to replace the instance ID with another solution.
In our case the remoteproc instance does not have a base address
to use. So, we can't put remoteproc@address
What do you recommend in this case please ?
Kind regards,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-19 9:35 ` Abdellatif El Khlifi
@ 2024-09-19 10:04 ` Krzysztof Kozlowski
2024-09-19 14:57 ` Abdellatif El Khlifi
2024-09-22 18:58 ` Krzysztof Kozlowski
1 sibling, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-19 10:04 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On 19/09/2024 11:35, Abdellatif El Khlifi wrote:
> Hi Krzysztof,
>
>>> Add devicetree binding schema for the External Systems remote processors
>>>
>>> The External Systems remote processors are provided on the Corstone-1000
>>> IoT Reference Design Platform via the SSE-710 subsystem.
>>>
>>> For more details about the External Systems, please see Corstone SSE-710
>>> subsystem features [1].
>>>
>>
>> Do not attach (thread) your patchsets to some other threads (unrelated
>> or older versions). This buries them deep in the mailbox and might
>> interfere with applying entire sets.
>>
>>> [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
>>>
>>> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>>> ---
>>> .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
>>> 1 file changed, 90 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>>> new file mode 100644
>>> index 000000000000..827ba8d962f1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>>> @@ -0,0 +1,90 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: SSE-710 External System Remote Processor
>>> +
>>> +maintainers:
>>> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>>> + - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
>>> +
>>> +description: |
>>
>> dt-preserve-formatting
>
> Do you mean I should remove the '|' please ? (I didn't find examples of use of
> dt-preserve-formatting in Documentation/devicetree/bindings/)
>
>>
>>> + SSE-710 is an heterogeneous subsystem supporting up to two remote
>>> + processors aka the External Systems.
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - arm,sse710-extsys
>>> +
>>> + firmware-name:
>>> + description:
>>> + The default name of the firmware to load to the remote processor.
>>> +
>>> + '#extsys-id':
>>
>> '#' is not correct for sure, that's not a cell specifier.
>>
>> But anyway, we do not accept in general instance IDs.
>
> I'm happy to replace the instance ID with another solution.
> In our case the remoteproc instance does not have a base address
> to use. So, we can't put remoteproc@address
>
> What do you recommend in this case please ?
Waiting one month to respond is a great way to drop all context from my
memory. The emails are not even available for me - gone from inbox.
Bus addressing could note it. Or you have different devices, so
different compatibles. Tricky to say, because you did not describe the
hardware really and it's one month later...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-19 10:04 ` Krzysztof Kozlowski
@ 2024-09-19 14:57 ` Abdellatif El Khlifi
2024-09-20 12:42 ` Krzysztof Kozlowski
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-09-19 14:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Hi Krzysztof,
> >>> + '#extsys-id':
> >>
> >> '#' is not correct for sure, that's not a cell specifier.
> >>
> >> But anyway, we do not accept in general instance IDs.
> >
> > I'm happy to replace the instance ID with another solution.
> > In our case the remoteproc instance does not have a base address
> > to use. So, we can't put remoteproc@address
> >
> > What do you recommend in this case please ?
>
> Waiting one month to respond is a great way to drop all context from my
> memory. The emails are not even available for me - gone from inbox.
>
> Bus addressing could note it. Or you have different devices, so
> different compatibles. Tricky to say, because you did not describe the
> hardware really and it's one month later...
>
Sorry for waiting. I was in holidays.
I'll add more documentation about the external system for more clarity [1].
Basically, Linux runs on the Cortex-A35. The External system is a
Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
It can only control Cortex-M core using the reset control and status registers mapped
in the memory space of the Cortex-A35.
I'll make sure this explanation is added to the binding and commit log for
more clarity.
Thanks for the suggestion regarding supporting multiple instances of the
External system. I will send a new version shortly addressing all comments.
[1]: paragraph 2.3, https://developer.arm.com/documentation/dai0550/D/?lang=en
Kind regards
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-19 14:57 ` Abdellatif El Khlifi
@ 2024-09-20 12:42 ` Krzysztof Kozlowski
2024-09-20 14:19 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-20 12:42 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On 19/09/2024 16:57, Abdellatif El Khlifi wrote:
> Hi Krzysztof,
>
>>>>> + '#extsys-id':
>>>>
>>>> '#' is not correct for sure, that's not a cell specifier.
>>>>
>>>> But anyway, we do not accept in general instance IDs.
>>>
>>> I'm happy to replace the instance ID with another solution.
>>> In our case the remoteproc instance does not have a base address
>>> to use. So, we can't put remoteproc@address
>>>
>>> What do you recommend in this case please ?
>>
>> Waiting one month to respond is a great way to drop all context from my
>> memory. The emails are not even available for me - gone from inbox.
>>
>> Bus addressing could note it. Or you have different devices, so
>> different compatibles. Tricky to say, because you did not describe the
>> hardware really and it's one month later...
>>
>
> Sorry for waiting. I was in holidays.
>
> I'll add more documentation about the external system for more clarity [1].
>
> Basically, Linux runs on the Cortex-A35. The External system is a
> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
> It can only control Cortex-M core using the reset control and status registers mapped
> in the memory space of the Cortex-A35.
That's pretty standard.
It does not explain me why bus addressing or different compatible are
not sufficient here.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-20 12:42 ` Krzysztof Kozlowski
@ 2024-09-20 14:19 ` Abdellatif El Khlifi
2024-09-20 14:56 ` Krzysztof Kozlowski
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-09-20 14:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Hi Krzysztof,
> >>>>> + '#extsys-id':
> >>>>
> >>>> '#' is not correct for sure, that's not a cell specifier.
> >>>>
> >>>> But anyway, we do not accept in general instance IDs.
> >>>
> >>> I'm happy to replace the instance ID with another solution.
> >>> In our case the remoteproc instance does not have a base address
> >>> to use. So, we can't put remoteproc@address
> >>>
> >>> What do you recommend in this case please ?
> >>
> >> Waiting one month to respond is a great way to drop all context from my
> >> memory. The emails are not even available for me - gone from inbox.
> >>
> >> Bus addressing could note it. Or you have different devices, so
> >> different compatibles. Tricky to say, because you did not describe the
> >> hardware really and it's one month later...
> >>
> >
> > Sorry for waiting. I was in holidays.
> >
> > I'll add more documentation about the external system for more clarity [1].
> >
> > Basically, Linux runs on the Cortex-A35. The External system is a
> > Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
> > It can only control Cortex-M core using the reset control and status registers mapped
> > in the memory space of the Cortex-A35.
>
> That's pretty standard.
>
> It does not explain me why bus addressing or different compatible are
> not sufficient here.
Using an instance ID was a design choice.
I'm happy to replace it with the use of compatible and match data (WIP).
The match data will be pointing to a data structure containing the right offsets
to be used with regmap APIs.
syscon node is used to represent the Host Base System Control register area [1]
where the external system reset registers are mapped (EXT_SYS*).
The nodes will look like this:
syscon@1a010000 {
compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
reg = <0x1a010000 0x1000>;
#address-cells = <1>;
#size-cells = <1>;
remoteproc@310 {
compatible = "arm,sse710-extsys0";
reg = <0x310 4>;
...
}
remoteproc@318 {
compatible = "arm,sse710-extsys1";
reg = <0x318 4>;
...
}
[1]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
Cheers
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-20 14:19 ` Abdellatif El Khlifi
@ 2024-09-20 14:56 ` Krzysztof Kozlowski
2024-09-20 16:38 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-20 14:56 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On 20/09/2024 16:19, Abdellatif El Khlifi wrote:
> Hi Krzysztof,
>
>>>>>>> + '#extsys-id':
>>>>>>
>>>>>> '#' is not correct for sure, that's not a cell specifier.
>>>>>>
>>>>>> But anyway, we do not accept in general instance IDs.
>>>>>
>>>>> I'm happy to replace the instance ID with another solution.
>>>>> In our case the remoteproc instance does not have a base address
>>>>> to use. So, we can't put remoteproc@address
>>>>>
>>>>> What do you recommend in this case please ?
>>>>
>>>> Waiting one month to respond is a great way to drop all context from my
>>>> memory. The emails are not even available for me - gone from inbox.
>>>>
>>>> Bus addressing could note it. Or you have different devices, so
>>>> different compatibles. Tricky to say, because you did not describe the
>>>> hardware really and it's one month later...
>>>>
>>>
>>> Sorry for waiting. I was in holidays.
>>>
>>> I'll add more documentation about the external system for more clarity [1].
>>>
>>> Basically, Linux runs on the Cortex-A35. The External system is a
>>> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
>>> It can only control Cortex-M core using the reset control and status registers mapped
>>> in the memory space of the Cortex-A35.
>>
>> That's pretty standard.
>>
>> It does not explain me why bus addressing or different compatible are
>> not sufficient here.
>
> Using an instance ID was a design choice.
> I'm happy to replace it with the use of compatible and match data (WIP).
>
> The match data will be pointing to a data structure containing the right offsets
> to be used with regmap APIs.
>
> syscon node is used to represent the Host Base System Control register area [1]
> where the external system reset registers are mapped (EXT_SYS*).
>
> The nodes will look like this:
>
> syscon@1a010000 {
> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> reg = <0x1a010000 0x1000>;
>
> #address-cells = <1>;
> #size-cells = <1>;
>
> remoteproc@310 {
> compatible = "arm,sse710-extsys0";
> reg = <0x310 4>;
Uh, why do you create device nodes for one word? This really suggests it
is part of parent device and your split is artificial.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-20 14:56 ` Krzysztof Kozlowski
@ 2024-09-20 16:38 ` Abdellatif El Khlifi
2024-09-21 18:20 ` Krzysztof Kozlowski
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-09-20 16:38 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Hi Krzysztof,
> >>>>>>> + '#extsys-id':
> >>>>>>
> >>>>>> '#' is not correct for sure, that's not a cell specifier.
> >>>>>>
> >>>>>> But anyway, we do not accept in general instance IDs.
> >>>>>
> >>>>> I'm happy to replace the instance ID with another solution.
> >>>>> In our case the remoteproc instance does not have a base address
> >>>>> to use. So, we can't put remoteproc@address
> >>>>>
> >>>>> What do you recommend in this case please ?
> >>>>
> >>>> Waiting one month to respond is a great way to drop all context from my
> >>>> memory. The emails are not even available for me - gone from inbox.
> >>>>
> >>>> Bus addressing could note it. Or you have different devices, so
> >>>> different compatibles. Tricky to say, because you did not describe the
> >>>> hardware really and it's one month later...
> >>>>
> >>>
> >>> Sorry for waiting. I was in holidays.
> >>>
> >>> I'll add more documentation about the external system for more clarity [1].
> >>>
> >>> Basically, Linux runs on the Cortex-A35. The External system is a
> >>> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
> >>> It can only control Cortex-M core using the reset control and status registers mapped
> >>> in the memory space of the Cortex-A35.
> >>
> >> That's pretty standard.
> >>
> >> It does not explain me why bus addressing or different compatible are
> >> not sufficient here.
> >
> > Using an instance ID was a design choice.
> > I'm happy to replace it with the use of compatible and match data (WIP).
> >
> > The match data will be pointing to a data structure containing the right offsets
> > to be used with regmap APIs.
> >
> > syscon node is used to represent the Host Base System Control register area [1]
> > where the external system reset registers are mapped (EXT_SYS*).
> >
> > The nodes will look like this:
> >
> > syscon@1a010000 {
> > compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> > reg = <0x1a010000 0x1000>;
> >
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > remoteproc@310 {
> > compatible = "arm,sse710-extsys0";
> > reg = <0x310 4>;
>
> Uh, why do you create device nodes for one word? This really suggests it
> is part of parent device and your split is artificial.
The external system registers (described by the remoteproc node) are part
of the parent device (the Host Base System Control register area) described
by syscon.
In case of the external system 0 , its registers are located at offset 0x310
(physical address: 0x1a010310)
When instantiating the devices without @address, the DTC compiler
detects 2 nodes with the same name (remoteproc).
syscon@1a010000 {
...
remoteproc {
compatible = "arm,sse710-extsys0";
...
}
remoteproc {
compatible = "arm,sse710-extsys1";
...
}
Cheers
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-20 16:38 ` Abdellatif El Khlifi
@ 2024-09-21 18:20 ` Krzysztof Kozlowski
2024-09-23 11:49 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-21 18:20 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On 20/09/2024 18:38, Abdellatif El Khlifi wrote:
> Hi Krzysztof,
>
>>>>>>>>> + '#extsys-id':
>>>>>>>>
>>>>>>>> '#' is not correct for sure, that's not a cell specifier.
>>>>>>>>
>>>>>>>> But anyway, we do not accept in general instance IDs.
>>>>>>>
>>>>>>> I'm happy to replace the instance ID with another solution.
>>>>>>> In our case the remoteproc instance does not have a base address
>>>>>>> to use. So, we can't put remoteproc@address
>>>>>>>
>>>>>>> What do you recommend in this case please ?
>>>>>>
>>>>>> Waiting one month to respond is a great way to drop all context from my
>>>>>> memory. The emails are not even available for me - gone from inbox.
>>>>>>
>>>>>> Bus addressing could note it. Or you have different devices, so
>>>>>> different compatibles. Tricky to say, because you did not describe the
>>>>>> hardware really and it's one month later...
>>>>>>
>>>>>
>>>>> Sorry for waiting. I was in holidays.
>>>>>
>>>>> I'll add more documentation about the external system for more clarity [1].
>>>>>
>>>>> Basically, Linux runs on the Cortex-A35. The External system is a
>>>>> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
>>>>> It can only control Cortex-M core using the reset control and status registers mapped
>>>>> in the memory space of the Cortex-A35.
>>>>
>>>> That's pretty standard.
>>>>
>>>> It does not explain me why bus addressing or different compatible are
>>>> not sufficient here.
>>>
>>> Using an instance ID was a design choice.
>>> I'm happy to replace it with the use of compatible and match data (WIP).
>>>
>>> The match data will be pointing to a data structure containing the right offsets
>>> to be used with regmap APIs.
>>>
>>> syscon node is used to represent the Host Base System Control register area [1]
>>> where the external system reset registers are mapped (EXT_SYS*).
>>>
>>> The nodes will look like this:
>>>
>>> syscon@1a010000 {
>>> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
>>> reg = <0x1a010000 0x1000>;
>>>
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>>
>>> remoteproc@310 {
>>> compatible = "arm,sse710-extsys0";
>>> reg = <0x310 4>;
>>
>> Uh, why do you create device nodes for one word? This really suggests it
>> is part of parent device and your split is artificial.
>
> The external system registers (described by the remoteproc node) are part
> of the parent device (the Host Base System Control register area) described
> by syscon.
>
> In case of the external system 0 , its registers are located at offset 0x310
> (physical address: 0x1a010310)
>
> When instantiating the devices without @address, the DTC compiler
> detects 2 nodes with the same name (remoteproc).
There should be no children at all. DT is not for instantiating your
drivers. I claim you have only one device and that's
arm,sse710-host-base-sysctrl. If you create child node for one word,
that's not a device.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-19 9:35 ` Abdellatif El Khlifi
2024-09-19 10:04 ` Krzysztof Kozlowski
@ 2024-09-22 18:58 ` Krzysztof Kozlowski
1 sibling, 0 replies; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-22 18:58 UTC (permalink / raw)
To: Abdellatif El Khlifi, Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On 19/09/2024 11:35, Abdellatif El Khlifi wrote:
> Hi Krzysztof,
>
>>> Add devicetree binding schema for the External Systems remote processors
>>>
>>> The External Systems remote processors are provided on the Corstone-1000
>>> IoT Reference Design Platform via the SSE-710 subsystem.
>>>
>>> For more details about the External Systems, please see Corstone SSE-710
>>> subsystem features [1].
>>>
>>
>> Do not attach (thread) your patchsets to some other threads (unrelated
>> or older versions). This buries them deep in the mailbox and might
>> interfere with applying entire sets.
>>
>>> [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
>>>
>>> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>>> ---
>>> .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
>>> 1 file changed, 90 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>>> new file mode 100644
>>> index 000000000000..827ba8d962f1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>>> @@ -0,0 +1,90 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: SSE-710 External System Remote Processor
>>> +
>>> +maintainers:
>>> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
>>> + - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
>>> +
>>> +description: |
>>
>> dt-preserve-formatting
>
> Do you mean I should remove the '|' please ? (I didn't find examples of use of
> dt-preserve-formatting in Documentation/devicetree/bindings/)
I am sorry, it was supposed to be expanded from VIM snippet, but I did
not finish the expansion. The point was to remove '|' because it is not
needed.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-21 18:20 ` Krzysztof Kozlowski
@ 2024-09-23 11:49 ` Abdellatif El Khlifi
2024-09-23 15:29 ` Krzysztof Kozlowski
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-09-23 11:49 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Hi Krzysztof,
> >>>>>>>>> + '#extsys-id':
> >>>>>>>>
> >>>>>>>> '#' is not correct for sure, that's not a cell specifier.
> >>>>>>>>
> >>>>>>>> But anyway, we do not accept in general instance IDs.
> >>>>>>>
> >>>>>>> I'm happy to replace the instance ID with another solution.
> >>>>>>> In our case the remoteproc instance does not have a base address
> >>>>>>> to use. So, we can't put remoteproc@address
> >>>>>>>
> >>>>>>> What do you recommend in this case please ?
> >>>>>>
> >>>>>> Waiting one month to respond is a great way to drop all context from my
> >>>>>> memory. The emails are not even available for me - gone from inbox.
> >>>>>>
> >>>>>> Bus addressing could note it. Or you have different devices, so
> >>>>>> different compatibles. Tricky to say, because you did not describe the
> >>>>>> hardware really and it's one month later...
> >>>>>>
> >>>>>
> >>>>> Sorry for waiting. I was in holidays.
> >>>>>
> >>>>> I'll add more documentation about the external system for more clarity [1].
> >>>>>
> >>>>> Basically, Linux runs on the Cortex-A35. The External system is a
> >>>>> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
> >>>>> It can only control Cortex-M core using the reset control and status registers mapped
> >>>>> in the memory space of the Cortex-A35.
> >>>>
> >>>> That's pretty standard.
> >>>>
> >>>> It does not explain me why bus addressing or different compatible are
> >>>> not sufficient here.
> >>>
> >>> Using an instance ID was a design choice.
> >>> I'm happy to replace it with the use of compatible and match data (WIP).
> >>>
> >>> The match data will be pointing to a data structure containing the right offsets
> >>> to be used with regmap APIs.
> >>>
> >>> syscon node is used to represent the Host Base System Control register area [1]
> >>> where the external system reset registers are mapped (EXT_SYS*).
> >>>
> >>> The nodes will look like this:
> >>>
> >>> syscon@1a010000 {
> >>> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> >>> reg = <0x1a010000 0x1000>;
> >>>
> >>> #address-cells = <1>;
> >>> #size-cells = <1>;
> >>>
> >>> remoteproc@310 {
> >>> compatible = "arm,sse710-extsys0";
> >>> reg = <0x310 4>;
> >>
> >> Uh, why do you create device nodes for one word? This really suggests it
> >> is part of parent device and your split is artificial.
> >
> > The external system registers (described by the remoteproc node) are part
> > of the parent device (the Host Base System Control register area) described
> > by syscon.
> >
> > In case of the external system 0 , its registers are located at offset 0x310
> > (physical address: 0x1a010310)
> >
> > When instantiating the devices without @address, the DTC compiler
> > detects 2 nodes with the same name (remoteproc).
>
> There should be no children at all. DT is not for instantiating your
> drivers. I claim you have only one device and that's
> arm,sse710-host-base-sysctrl. If you create child node for one word,
> that's not a device.
The Host Base System Control [3] is the big block containing various functionalities (MMIO registers).
Among the functionalities, the two remote cores registers (aka External system 0 and 1).
The remote cores have two registers each.
1/ In the v1 patchset, a valid point was made by the community:
Right now it seems somewhat tenuous to describe two consecutive
32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
all there ever is. However if it's actually going to end up needing several
more additional MMIO and/or memory regions for other functionality, then
describing each register and location individually is liable to get
unmanageable really fast, and a higher-level functional grouping (e.g. these
reset-related registers together as a single 8-byte region) would likely be
a better design.
The Exernal system registers are part of a bigger block with other functionality in place.
MFD/syscon might be better way to use these registers. You never know in
future you might want to use another set of 2-4 registers with a different
functionality in another driver.
I would see if it makes sense to put together a single binding for
this "Host Base System Control" register (not sure what exactly that means).
Use MFD/regmap you access parts of this block. The remoteproc driver can
then be semi-generic (meaning applicable to group of similar platforms)
based on the platform compatible and use this regmap to provide the
functionality needed.
2/ There are many examples in the kernel that use syscon as a parent node of
child nodes for devices located at an offset from the syscon base address.
Please see these two examples [1][2]. I'm trying to follow a similar design if that
makes sense.
3/ Since there are two registers for each remote core. I'm suggesting to set the
size in the reg property to 8. The driver will read the match data to get the right
offset to be used with regmap APIs.
Suggested nodes:
syscon@1a010000 {
compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
reg = <0x1a010000 0x1000>;
#address-cells = <1>;
#size-cells = <1>;
remoteproc@310 {
compatible = "arm,sse710-extsys0";
reg = <0x310 8>;
firmware-name = "es_flashfw.elf";
mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
mbox-names = "txes0", "rxes0";
memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
};
remoteproc@318 {
compatible = "arm,sse710-extsys1";
reg = <0x318 8>;
firmware-name = "es_flashfw.elf";
mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
mbox-names = "txes0", "rxes0";
memory-region = <&extsys1_vring0>, <&extsys1_vring1>;
};
};
[1]: Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
syscon@20e00000 {
compatible = "sprd,sc9863a-glbregs", "syscon", "simple-mfd";
reg = <0x20e00000 0x4000>;
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x20e00000 0x4000>;
apahb_gate: apahb-gate@0 {
compatible = "sprd,sc9863a-apahb-gate";
reg = <0x0 0x1020>;
#clock-cells = <1>;
};
};
[2]: Documentation/devicetree/bindings/arm/arm,juno-fpga-apb-regs.yaml:
syscon@10000 {
compatible = "arm,juno-fpga-apb-regs", "syscon", "simple-mfd";
reg = <0x010000 0x1000>;
ranges = <0x0 0x10000 0x1000>;
#address-cells = <1>;
#size-cells = <1>;
led@8,0 {
compatible = "register-bit-led";
reg = <0x08 0x04>;
offset = <0x08>;
mask = <0x01>;
label = "vexpress:0";
linux,default-trigger = "heartbeat";
default-state = "on";
};
};
[3]: https://developer.arm.com/documentation/102342/0000/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-23 11:49 ` Abdellatif El Khlifi
@ 2024-09-23 15:29 ` Krzysztof Kozlowski
2024-09-23 17:19 ` Abdellatif El Khlifi
0 siblings, 1 reply; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-23 15:29 UTC (permalink / raw)
To: Abdellatif El Khlifi, Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On 23/09/2024 13:49, Abdellatif El Khlifi wrote:
> Hi Krzysztof,
>
>>>>>>>>>>> + '#extsys-id':
>>>>>>>>>>
>>>>>>>>>> '#' is not correct for sure, that's not a cell specifier.
>>>>>>>>>>
>>>>>>>>>> But anyway, we do not accept in general instance IDs.
>>>>>>>>>
>>>>>>>>> I'm happy to replace the instance ID with another solution.
>>>>>>>>> In our case the remoteproc instance does not have a base address
>>>>>>>>> to use. So, we can't put remoteproc@address
>>>>>>>>>
>>>>>>>>> What do you recommend in this case please ?
>>>>>>>>
>>>>>>>> Waiting one month to respond is a great way to drop all context from my
>>>>>>>> memory. The emails are not even available for me - gone from inbox.
>>>>>>>>
>>>>>>>> Bus addressing could note it. Or you have different devices, so
>>>>>>>> different compatibles. Tricky to say, because you did not describe the
>>>>>>>> hardware really and it's one month later...
>>>>>>>>
>>>>>>>
>>>>>>> Sorry for waiting. I was in holidays.
>>>>>>>
>>>>>>> I'll add more documentation about the external system for more clarity [1].
>>>>>>>
>>>>>>> Basically, Linux runs on the Cortex-A35. The External system is a
>>>>>>> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
>>>>>>> It can only control Cortex-M core using the reset control and status registers mapped
>>>>>>> in the memory space of the Cortex-A35.
>>>>>>
>>>>>> That's pretty standard.
>>>>>>
>>>>>> It does not explain me why bus addressing or different compatible are
>>>>>> not sufficient here.
>>>>>
>>>>> Using an instance ID was a design choice.
>>>>> I'm happy to replace it with the use of compatible and match data (WIP).
>>>>>
>>>>> The match data will be pointing to a data structure containing the right offsets
>>>>> to be used with regmap APIs.
>>>>>
>>>>> syscon node is used to represent the Host Base System Control register area [1]
>>>>> where the external system reset registers are mapped (EXT_SYS*).
>>>>>
>>>>> The nodes will look like this:
>>>>>
>>>>> syscon@1a010000 {
>>>>> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
>>>>> reg = <0x1a010000 0x1000>;
>>>>>
>>>>> #address-cells = <1>;
>>>>> #size-cells = <1>;
>>>>>
>>>>> remoteproc@310 {
>>>>> compatible = "arm,sse710-extsys0";
>>>>> reg = <0x310 4>;
>>>>
>>>> Uh, why do you create device nodes for one word? This really suggests it
>>>> is part of parent device and your split is artificial.
>>>
>>> The external system registers (described by the remoteproc node) are part
>>> of the parent device (the Host Base System Control register area) described
>>> by syscon.
>>>
>>> In case of the external system 0 , its registers are located at offset 0x310
>>> (physical address: 0x1a010310)
>>>
>>> When instantiating the devices without @address, the DTC compiler
>>> detects 2 nodes with the same name (remoteproc).
>>
>> There should be no children at all. DT is not for instantiating your
>> drivers. I claim you have only one device and that's
>> arm,sse710-host-base-sysctrl. If you create child node for one word,
>> that's not a device.
>
> The Host Base System Control [3] is the big block containing various functionalities (MMIO registers).
> Among the functionalities, the two remote cores registers (aka External system 0 and 1).
> The remote cores have two registers each.
>
> 1/ In the v1 patchset, a valid point was made by the community:
>
> Right now it seems somewhat tenuous to describe two consecutive
> 32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
ARM is not special, neither this hardware is. Therefore:
1. Each register as reg: nope, for obvious reasons.
2. One device for entire syscon: quite common, why do you think it is
somehow odd?
3. If you quote other person, please provide the lore link, so I won't
spend useless 5 minutes to find who said that or if it was even said...
> all there ever is. However if it's actually going to end up needing several
> more additional MMIO and/or memory regions for other functionality, then
> describing each register and location individually is liable to get
> unmanageable really fast, and a higher-level functional grouping (e.g. these
> reset-related registers together as a single 8-byte region) would likely be
> a better design.
>
> The Exernal system registers are part of a bigger block with other functionality in place.
> MFD/syscon might be better way to use these registers. You never know in
> future you might want to use another set of 2-4 registers with a different
> functionality in another driver.
>
> I would see if it makes sense to put together a single binding for
> this "Host Base System Control" register (not sure what exactly that means).
> Use MFD/regmap you access parts of this block. The remoteproc driver can
> then be semi-generic (meaning applicable to group of similar platforms)
> based on the platform compatible and use this regmap to provide the
> functionality needed.
I don't understand how this lengthy semi-quote answers my concerns.
Please write concise points as arguments to my questions.
For example, I don't care what your remote proc driver does and it
should not matter in the terms of this binding.
>
> 2/ There are many examples in the kernel that use syscon as a parent node of
> child nodes for devices located at an offset from the syscon base address.
> Please see these two examples [1][2]. I'm trying to follow a similar design if that
> makes sense.
Yeah, for separate devices. If you have two words without any resources,
I claim you might not have here any separate devices or "not separate
enough", because all this is somehow fluid. Remote core sounds like
separate device, but all your arguments about need of extid which cannot
be used in reg does not support this case.
The example in the binding is also not complete - missing rest of
devices - which does not help.
>
> 3/ Since there are two registers for each remote core. I'm suggesting to set the
> size in the reg property to 8.
How is this related?
> The driver will read the match data to get the right
> offset to be used with regmap APIs.
Sorry, no talks about driver. Don't care, at least in this context.
You can completely omit address space from children in DT and everything
will work fine and look fine from bindings point of view.
>
> Suggested nodes:
>
>
> syscon@1a010000 {
> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> reg = <0x1a010000 0x1000>;
>
> #address-cells = <1>;
> #size-cells = <1>;
>
> remoteproc@310 {
> compatible = "arm,sse710-extsys0";
> reg = <0x310 8>;
> firmware-name = "es_flashfw.elf";
> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
> mbox-names = "txes0", "rxes0";
> memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
> };
>
> remoteproc@318 {
> compatible = "arm,sse710-extsys1";
> reg = <0x318 8>;
> firmware-name = "es_flashfw.elf";
Same firmware? Always or only depends?
> mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
> mbox-names = "txes0", "rxes0";
> memory-region = <&extsys1_vring0>, <&extsys1_vring1>;
The rest of resources support the idea of two children but I still have
doubts about need of identifying remote instances.
Looking at your driver it is totally not needed. Your driver just
duplicates the regs here, so it's a proof that you are not using DT
correctly.
> };
> };
>
>
> [1]: Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
>
> syscon@20e00000 {
> compatible = "sprd,sc9863a-glbregs", "syscon", "simple-mfd";
> reg = <0x20e00000 0x4000>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0 0x20e00000 0x4000>;
>
> apahb_gate: apahb-gate@0 {
> compatible = "sprd,sc9863a-apahb-gate";
> reg = <0x0 0x1020>;
Well, size 1020, but please never use sprd as an example. You can as
well point to a buggy code and say that "I can implement bugs as well,
because there are bugs already".
Same for few other almost abandoned, poorly maintained platforms.
> #clock-cells = <1>;
> };
> };
>
>
> [2]: Documentation/devicetree/bindings/arm/arm,juno-fpga-apb-regs.yaml:
>
> syscon@10000 {
> compatible = "arm,juno-fpga-apb-regs", "syscon", "simple-mfd";
> reg = <0x010000 0x1000>;
> ranges = <0x0 0x10000 0x1000>;
> #address-cells = <1>;
> #size-cells = <1>;
>
> led@8,0 {
> compatible = "register-bit-led";
register-bit-led... what do you want to prove? You will find
clocks-per-reg and try to implement them? That's known no-go.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-23 15:29 ` Krzysztof Kozlowski
@ 2024-09-23 17:19 ` Abdellatif El Khlifi
2024-09-27 7:57 ` Krzysztof Kozlowski
0 siblings, 1 reply; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-09-23 17:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
Hi Krzysztof,
> >>>>>>>>>>> + '#extsys-id':
> >>>>>>>>>>
> >>>>>>>>>> '#' is not correct for sure, that's not a cell specifier.
> >>>>>>>>>>
> >>>>>>>>>> But anyway, we do not accept in general instance IDs.
> >>>>>>>>>
> >>>>>>>>> I'm happy to replace the instance ID with another solution.
> >>>>>>>>> In our case the remoteproc instance does not have a base address
> >>>>>>>>> to use. So, we can't put remoteproc@address
> >>>>>>>>>
> >>>>>>>>> What do you recommend in this case please ?
> >>>>>>>>
> >>>>>>>> Waiting one month to respond is a great way to drop all context from my
> >>>>>>>> memory. The emails are not even available for me - gone from inbox.
> >>>>>>>>
> >>>>>>>> Bus addressing could note it. Or you have different devices, so
> >>>>>>>> different compatibles. Tricky to say, because you did not describe the
> >>>>>>>> hardware really and it's one month later...
> >>>>>>>>
> >>>>>>>
> >>>>>>> Sorry for waiting. I was in holidays.
> >>>>>>>
> >>>>>>> I'll add more documentation about the external system for more clarity [1].
> >>>>>>>
> >>>>>>> Basically, Linux runs on the Cortex-A35. The External system is a
> >>>>>>> Cortex-M core. The Cortex-A35 can not access the memory of the Cortex-M.
> >>>>>>> It can only control Cortex-M core using the reset control and status registers mapped
> >>>>>>> in the memory space of the Cortex-A35.
> >>>>>>
> >>>>>> That's pretty standard.
> >>>>>>
> >>>>>> It does not explain me why bus addressing or different compatible are
> >>>>>> not sufficient here.
> >>>>>
> >>>>> Using an instance ID was a design choice.
> >>>>> I'm happy to replace it with the use of compatible and match data (WIP).
> >>>>>
> >>>>> The match data will be pointing to a data structure containing the right offsets
> >>>>> to be used with regmap APIs.
> >>>>>
> >>>>> syscon node is used to represent the Host Base System Control register area [1]
> >>>>> where the external system reset registers are mapped (EXT_SYS*).
> >>>>>
> >>>>> The nodes will look like this:
> >>>>>
> >>>>> syscon@1a010000 {
> >>>>> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> >>>>> reg = <0x1a010000 0x1000>;
> >>>>>
> >>>>> #address-cells = <1>;
> >>>>> #size-cells = <1>;
> >>>>>
> >>>>> remoteproc@310 {
> >>>>> compatible = "arm,sse710-extsys0";
> >>>>> reg = <0x310 4>;
> >>>>
> >>>> Uh, why do you create device nodes for one word? This really suggests it
> >>>> is part of parent device and your split is artificial.
> >>>
> >>> The external system registers (described by the remoteproc node) are part
> >>> of the parent device (the Host Base System Control register area) described
> >>> by syscon.
> >>>
> >>> In case of the external system 0 , its registers are located at offset 0x310
> >>> (physical address: 0x1a010310)
> >>>
> >>> When instantiating the devices without @address, the DTC compiler
> >>> detects 2 nodes with the same name (remoteproc).
> >>
> >> There should be no children at all. DT is not for instantiating your
> >> drivers. I claim you have only one device and that's
> >> arm,sse710-host-base-sysctrl. If you create child node for one word,
> >> that's not a device.
> >
> > The Host Base System Control [3] is the big block containing various functionalities (MMIO registers).
> > Among the functionalities, the two remote cores registers (aka External system 0 and 1).
> > The remote cores have two registers each.
> >
> > 1/ In the v1 patchset, a valid point was made by the community:
> >
> > Right now it seems somewhat tenuous to describe two consecutive
> > 32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
>
> ARM is not special, neither this hardware is. Therefore:
> 1. Each register as reg: nope, for obvious reasons.
> 2. One device for entire syscon: quite common, why do you think it is
> somehow odd?
> 3. If you quote other person, please provide the lore link, so I won't
> spend useless 5 minutes to find who said that or if it was even said...
Please have a look at this lore link [1]. The idea is to add syscon
and regmap support which I did in the v2 patchset.
[1]: https://lore.kernel.org/all/ZfMVcQsmgQUXXcef@bogus/
>
> > all there ever is. However if it's actually going to end up needing several
> > more additional MMIO and/or memory regions for other functionality, then
> > describing each register and location individually is liable to get
> > unmanageable really fast, and a higher-level functional grouping (e.g. these
> > reset-related registers together as a single 8-byte region) would likely be
> > a better design.
> >
> > The Exernal system registers are part of a bigger block with other functionality in place.
> > MFD/syscon might be better way to use these registers. You never know in
> > future you might want to use another set of 2-4 registers with a different
> > functionality in another driver.
> >
> > I would see if it makes sense to put together a single binding for
> > this "Host Base System Control" register (not sure what exactly that means).
> > Use MFD/regmap you access parts of this block. The remoteproc driver can
> > then be semi-generic (meaning applicable to group of similar platforms)
> > based on the platform compatible and use this regmap to provide the
> > functionality needed.
>
> I don't understand how this lengthy semi-quote answers my concerns.
> Please write concise points as arguments to my questions.
>
> For example, I don't care what your remote proc driver does and it
> should not matter in the terms of this binding.
I just wanted to show why we are using syscon based on the arguments
of other reviewers.
>
> >
> > 2/ There are many examples in the kernel that use syscon as a parent node of
> > child nodes for devices located at an offset from the syscon base address.
> > Please see these two examples [1][2]. I'm trying to follow a similar design if that
> > makes sense.
>
> Yeah, for separate devices. If you have two words without any resources,
> I claim you might not have here any separate devices or "not separate
> enough", because all this is somehow fluid. Remote core sounds like
> separate device, but all your arguments about need of extid which cannot
> be used in reg does not support this case.
>
> The example in the binding is also not complete - missing rest of
> devices - which does not help.
Here I would like to explain the current suggestion and suggest an alternative solution.
1/ For more clarity, here is a complete example of use of both remote cores
at the same time:
syscon@1a010000 {
compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
reg = <0x1a010000 0x1000>;
#address-cells = <1>;
#size-cells = <1>;
remoteproc@310 {
compatible = "arm,sse710-extsys0";
reg = <0x310 8>;
firmware-name = "es0_flashfw.elf";
mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
mbox-names = "txes0", "rxes0";
memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
};
remoteproc@318 {
compatible = "arm,sse710-extsys1";
reg = <0x318 8>;
firmware-name = "es1_flashfw.elf";
mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
mbox-names = "txes0", "rxes0";
memory-region = <&extsys1_vring0>, <&extsys1_vring1>;
};
};
Here we have 2 cores, each one have 2 registers mapped respectively
at 0x1a010310 and 0x1a010318.
Definetly these cores have seperate HW resources associated with them
which are the MHUs (mailboxes HW). There are 2 pairs of MHUs associated
with each core. These mailbox peripherals are obviously seperate.
Furthermore, the vring buffers used for communication are seperate.
Moreover, the remote cores are independent. They are not SMP cores of one processor.
They can have different default firmware to use. In this example es0_flashfw.elf
and es1_flashfw.elf
The current HW implementation (Corstone-1000) provides one remote core only.
However, the second core control registers are at 0x1a010318 do exist.
Support for a second core is taken into consideration in this work to help
end users who want to add a second core to their HW.
2/ Here I'm suggesting an alternative solution by using one remoteproc node for both cores as
follows:
syscon@1a010000 {
compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
reg = <0x1a010000 0x1000>;
remoteproc {
compatible = "arm,sse710-extsys";
firmware-name = "es0_flashfw.elf";
mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>, <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
mbox-names = "txes0", "rxes0", "txes1", "rxes1";
memory-region = <&extsys0_vring0>, <&extsys0_vring1>, <&extsys1_vring0>, <&extsys1_vring1>;
};
};
Does this meet your expectations please ?
>
> >
> > 3/ Since there are two registers for each remote core. I'm suggesting to set the
> > size in the reg property to 8.
>
> How is this related?
>
> > The driver will read the match data to get the right
> > offset to be used with regmap APIs.
>
> Sorry, no talks about driver. Don't care, at least in this context.
>
> You can completely omit address space from children in DT and everything
> will work fine and look fine from bindings point of view.
>
> >
> > Suggested nodes:
> >
> >
> > syscon@1a010000 {
> > compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> > reg = <0x1a010000 0x1000>;
> >
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > remoteproc@310 {
> > compatible = "arm,sse710-extsys0";
> > reg = <0x310 8>;
> > firmware-name = "es_flashfw.elf";
> > mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
> > mbox-names = "txes0", "rxes0";
> > memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
> > };
> >
> > remoteproc@318 {
> > compatible = "arm,sse710-extsys1";
> > reg = <0x318 8>;
> > firmware-name = "es_flashfw.elf";
>
> Same firmware? Always or only depends?
The firmware of the second core depends on the end user choice.
Currently the second core is not implemented in the HW.
Users who want to tweak Corstone-1000 HW can add
a second core.
Kind regards
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-23 17:19 ` Abdellatif El Khlifi
@ 2024-09-27 7:57 ` Krzysztof Kozlowski
0 siblings, 0 replies; 59+ messages in thread
From: Krzysztof Kozlowski @ 2024-09-27 7:57 UTC (permalink / raw)
To: Abdellatif El Khlifi
Cc: mathieu.poirier, Adam.Johnston, Hugues.KambaMpiana, Drew.Reed,
andersson, conor+dt, devicetree, krzysztof.kozlowski+dt,
linux-arm-kernel, linux-kernel, linux-remoteproc, liviu.dudau,
lpieralisi, robh, sudeep.holla, robin.murphy
On 23/09/2024 19:19, Abdellatif El Khlifi wrote:
>>>
>>> The Host Base System Control [3] is the big block containing various functionalities (MMIO registers).
>>> Among the functionalities, the two remote cores registers (aka External system 0 and 1).
>>> The remote cores have two registers each.
>>>
>>> 1/ In the v1 patchset, a valid point was made by the community:
>>>
>>> Right now it seems somewhat tenuous to describe two consecutive
>>> 32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
>>
>> ARM is not special, neither this hardware is. Therefore:
>> 1. Each register as reg: nope, for obvious reasons.
>> 2. One device for entire syscon: quite common, why do you think it is
>> somehow odd?
>> 3. If you quote other person, please provide the lore link, so I won't
>> spend useless 5 minutes to find who said that or if it was even said...
>
> Please have a look at this lore link [1]. The idea is to add syscon
> and regmap support which I did in the v2 patchset.
>
> [1]: https://lore.kernel.org/all/ZfMVcQsmgQUXXcef@bogus/
There is nothing there about DT bindings. We do not talk here about
drivers. MFD and regmap are Linux driver stuff, not bindings.
I said nothing about not using MFD, regmap or whatever driver stuff you
want. We talk *only* about bindings. Syscon is mentioned there but I am
sorry - that quite a stretch to call a block syscon just because you
want regmap.
>
>>
>>> all there ever is. However if it's actually going to end up needing several
>>> more additional MMIO and/or memory regions for other functionality, then
>>> describing each register and location individually is liable to get
>>> unmanageable really fast, and a higher-level functional grouping (e.g. these
>>> reset-related registers together as a single 8-byte region) would likely be
>>> a better design.
>>>
>>> The Exernal system registers are part of a bigger block with other functionality in place.
>>> MFD/syscon might be better way to use these registers. You never know in
>>> future you might want to use another set of 2-4 registers with a different
>>> functionality in another driver.
>>>
>>> I would see if it makes sense to put together a single binding for
>>> this "Host Base System Control" register (not sure what exactly that means).
>>> Use MFD/regmap you access parts of this block. The remoteproc driver can
>>> then be semi-generic (meaning applicable to group of similar platforms)
>>> based on the platform compatible and use this regmap to provide the
>>> functionality needed.
>>
>> I don't understand how this lengthy semi-quote answers my concerns.
>> Please write concise points as arguments to my questions.
>>
>> For example, I don't care what your remote proc driver does and it
>> should not matter in the terms of this binding.
>
> I just wanted to show why we are using syscon based on the arguments
> of other reviewers.
>
>>
>>>
>>> 2/ There are many examples in the kernel that use syscon as a parent node of
>>> child nodes for devices located at an offset from the syscon base address.
>>> Please see these two examples [1][2]. I'm trying to follow a similar design if that
>>> makes sense.
>>
>> Yeah, for separate devices. If you have two words without any resources,
>> I claim you might not have here any separate devices or "not separate
>> enough", because all this is somehow fluid. Remote core sounds like
>> separate device, but all your arguments about need of extid which cannot
>> be used in reg does not support this case.
>>
>> The example in the binding is also not complete - missing rest of
>> devices - which does not help.
>
> Here I would like to explain the current suggestion and suggest an alternative solution.
>
> 1/ For more clarity, here is a complete example of use of both remote cores
> at the same time:
>
> syscon@1a010000 {
> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> reg = <0x1a010000 0x1000>;
>
> #address-cells = <1>;
> #size-cells = <1>;
>
> remoteproc@310 {
> compatible = "arm,sse710-extsys0";
> reg = <0x310 8>;
> firmware-name = "es0_flashfw.elf";
> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
> mbox-names = "txes0", "rxes0";
> memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
> };
>
> remoteproc@318 {
> compatible = "arm,sse710-extsys1";
> reg = <0x318 8>;
> firmware-name = "es1_flashfw.elf";
> mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
> mbox-names = "txes0", "rxes0";
> memory-region = <&extsys1_vring0>, <&extsys1_vring1>;
> };
> };
>
> Here we have 2 cores, each one have 2 registers mapped respectively
> at 0x1a010310 and 0x1a010318.
All this looks fine, resources are indeed reasonable, except that I
still do not understand why do you need to call them 0 and 1 (now via
compatible).
Your driver code shows this nicely - it is entirely redundant! The 'reg'
- so the base - is already there! You just duplicate it with the
extsys_id, instead of relying on the base. So think what is the point of
'reg' property if you do not use it?
>
> Definetly these cores have seperate HW resources associated with them
> which are the MHUs (mailboxes HW). There are 2 pairs of MHUs associated
> with each core. These mailbox peripherals are obviously seperate.
> Furthermore, the vring buffers used for communication are seperate.
>
> Moreover, the remote cores are independent. They are not SMP cores of one processor.
>
> They can have different default firmware to use. In this example es0_flashfw.elf
> and es1_flashfw.elf
>
> The current HW implementation (Corstone-1000) provides one remote core only.
> However, the second core control registers are at 0x1a010318 do exist.
>
> Support for a second core is taken into consideration in this work to help
> end users who want to add a second core to their HW.
>
> 2/ Here I'm suggesting an alternative solution by using one remoteproc node for both cores as
> follows:
>
> syscon@1a010000 {
> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> reg = <0x1a010000 0x1000>;
>
> remoteproc {
> compatible = "arm,sse710-extsys";
> firmware-name = "es0_flashfw.elf";
> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>, <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
> mbox-names = "txes0", "rxes0", "txes1", "rxes1";
> memory-region = <&extsys0_vring0>, <&extsys0_vring1>, <&extsys1_vring0>, <&extsys1_vring1>;
> };
> };
>
> Does this meet your expectations please ?
>
>>
>>>
>>> 3/ Since there are two registers for each remote core. I'm suggesting to set the
>>> size in the reg property to 8.
>>
>> How is this related?
>>
>>> The driver will read the match data to get the right
>>> offset to be used with regmap APIs.
>>
>> Sorry, no talks about driver. Don't care, at least in this context.
>>
>> You can completely omit address space from children in DT and everything
>> will work fine and look fine from bindings point of view.
>>
>>>
>>> Suggested nodes:
>>>
>>>
>>> syscon@1a010000 {
>>> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
>>> reg = <0x1a010000 0x1000>;
>>>
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>>
>>> remoteproc@310 {
>>> compatible = "arm,sse710-extsys0";
>>> reg = <0x310 8>;
>>> firmware-name = "es_flashfw.elf";
>>> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
>>> mbox-names = "txes0", "rxes0";
>>> memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
>>> };
>>>
>>> remoteproc@318 {
>>> compatible = "arm,sse710-extsys1";
>>> reg = <0x318 8>;
>>> firmware-name = "es_flashfw.elf";
>>
>> Same firmware? Always or only depends?
>
> The firmware of the second core depends on the end user choice.
> Currently the second core is not implemented in the HW.
> Users who want to tweak Corstone-1000 HW can add
> a second core.
Two cores make more sense in such case.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-08-22 17:09 ` [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors Abdellatif El Khlifi
2024-08-22 18:25 ` Rob Herring (Arm)
2024-08-23 6:23 ` Krzysztof Kozlowski
@ 2024-09-27 17:54 ` Robin Murphy
2024-10-09 9:46 ` Abdellatif El Khlifi
2 siblings, 1 reply; 59+ messages in thread
From: Robin Murphy @ 2024-09-27 17:54 UTC (permalink / raw)
To: Abdellatif El Khlifi, mathieu.poirier
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, andersson, conor+dt,
devicetree, krzysztof.kozlowski+dt, linux-arm-kernel,
linux-kernel, linux-remoteproc, liviu.dudau, lpieralisi, robh,
sudeep.holla
On 22/08/2024 6:09 pm, Abdellatif El Khlifi wrote:
> Add devicetree binding schema for the External Systems remote processors
>
> The External Systems remote processors are provided on the Corstone-1000
> IoT Reference Design Platform via the SSE-710 subsystem.
>
> For more details about the External Systems, please see Corstone SSE-710
> subsystem features [1].
>
> [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> new file mode 100644
> index 000000000000..827ba8d962f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SSE-710 External System Remote Processor
Thing is, this is not describing SSE-710. As far as I can work out, it
is describing the firmware and hardware that a particular example
implementation of the Corstone-1000 kit has chosen to put in the
"external system" hole in the SSE-710 within that kit.
If I license SSE-710 alone or even the Corstone-1000 kit, I can put
whatever I want in *my* implementation of those subsystems, so there
clearly cannot possibly be a common binding for that.
For instance what if I decide to combine a Cortex-M core plus a radio
and some other glue as my external subsystem? Do we have dozens of
remoteproc bindings and drivers for weird fixed-function remoteprocs
whose "firmware-name" implies a Bluetooth protocol stack? No, we treat
them as Bluetooth controller devices. Look at
devicetree/bindings/sound/fsl,rpmsg.yaml - it's even unashamedly an
rpmsg client, but it's still not abusing the remoteproc subsystem
because its function to the host OS is as an audio controller, not an
arbitrarily configurable processor.
As I said before, all SSE-710 actually implements is a reset mechanism,
so it only seems logical to model it as a reset controller, e.g.
something like:
hbsys: syscon@xyz {
compatible = "arm,sse710-host-base-sysctrl", "syscon";
reg = <xyz>;
#reset-cells = <1>;
};
something {
...
resets = <&hbsys 0>;
};
something-else {
...
resets = <&hbsys 1>;
};
Then if there is actually any meaningful functionality in the default
extsys0 firmware preloaded on the FPGA setup then define a binding for
"arm,corstone1000-an550-extsys0" to describe whatever that actually
does. If a user chooses to create and load their own different firmware,
they're going to need their own binding and driver for whatever *that*
firmware does.
FWIW, driver-wise the mapping to the reset API seems straightforward -
.assert hits RST_REQ, .deassert clears CPUWAIT (.status is possibly a
combination of CPUWAIT and RST_ACK?)
Thanks,
Robin.
> +
> +maintainers:
> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> + - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
> +
> +description: |
> + SSE-710 is an heterogeneous subsystem supporting up to two remote
> + processors aka the External Systems.
> +
> +properties:
> + compatible:
> + enum:
> + - arm,sse710-extsys
> +
> + firmware-name:
> + description:
> + The default name of the firmware to load to the remote processor.
> +
> + '#extsys-id':
> + description:
> + The External System ID.
> + enum: [0, 1]
> +
> + mbox-names:
> + items:
> + - const: txes0
> + - const: rxes0
> +
> + mboxes:
> + description:
> + The list of Message Handling Unit (MHU) channels used for bidirectional
> + communication. This property is only required if the virtio-based Rpmsg
> + messaging bus is used. For more details see the Arm MHUv2 Mailbox
> + Controller at devicetree/bindings/mailbox/arm,mhuv2.yaml
> +
> + minItems: 2
> + maxItems: 2
> +
> + memory-region:
> + description:
> + If present, a phandle for a reserved memory area that used for vdev
> + buffer, resource table, vring region and others used by the remote
> + processor.
> + minItems: 2
> + maxItems: 32
> +
> +required:
> + - compatible
> + - firmware-name
> + - '#extsys-id'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + reserved-memory {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + extsys0_vring0: vdev0vring0@82001000 {
> + reg = <0 0x82001000 0 0x8000>;
> + no-map;
> + };
> +
> + extsys0_vring1: vdev0vring1@82009000 {
> + reg = <0 0x82009000 0 0x8000>;
> + no-map;
> + };
> + };
> +
> + syscon@1a010000 {
> + compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> + reg = <0x1a010000 0x1000>;
> +
> + extsys0 {
> + compatible = "arm,sse710-extsys";
> + #extsys-id = <0>;
> + firmware-name = "es_flashfw.elf";
> + mbox-names = "txes0", "rxes0";
> + mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
> + memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
> + };
> + };
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors
2024-09-27 17:54 ` Robin Murphy
@ 2024-10-09 9:46 ` Abdellatif El Khlifi
0 siblings, 0 replies; 59+ messages in thread
From: Abdellatif El Khlifi @ 2024-10-09 9:46 UTC (permalink / raw)
To: mathieu.poirier, krzysztof.kozlowski+dt, robh, robin.murphy
Cc: Adam.Johnston, Hugues.KambaMpiana, Drew.Reed, andersson, conor+dt,
devicetree, linux-arm-kernel, linux-kernel, linux-remoteproc,
liviu.dudau, lpieralisi, sudeep.holla
Hello folks,
> On 22/08/2024 6:09 pm, Abdellatif El Khlifi wrote:
> > Add devicetree binding schema for the External Systems remote processors
> >
> > The External Systems remote processors are provided on the Corstone-1000
> > IoT Reference Design Platform via the SSE-710 subsystem.
> >
> > For more details about the External Systems, please see Corstone SSE-710
> > subsystem features [1].
> >
> > [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > ---
> > .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
> > 1 file changed, 90 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> > new file mode 100644
> > index 000000000000..827ba8d962f1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SSE-710 External System Remote Processor
>
> Thing is, this is not describing SSE-710. As far as I can work out, it is
> describing the firmware and hardware that a particular example
> implementation of the Corstone-1000 kit has chosen to put in the "external
> system" hole in the SSE-710 within that kit.
>
> If I license SSE-710 alone or even the Corstone-1000 kit, I can put whatever
> I want in *my* implementation of those subsystems, so there clearly cannot
> possibly be a common binding for that.
>
> For instance what if I decide to combine a Cortex-M core plus a radio and
> some other glue as my external subsystem? Do we have dozens of remoteproc
> bindings and drivers for weird fixed-function remoteprocs whose
> "firmware-name" implies a Bluetooth protocol stack? No, we treat them as
> Bluetooth controller devices. Look at
> devicetree/bindings/sound/fsl,rpmsg.yaml - it's even unashamedly an rpmsg
> client, but it's still not abusing the remoteproc subsystem because its
> function to the host OS is as an audio controller, not an arbitrarily
> configurable processor.
>
> As I said before, all SSE-710 actually implements is a reset mechanism, so
> it only seems logical to model it as a reset controller, e.g. something
> like:
>
> hbsys: syscon@xyz {
> compatible = "arm,sse710-host-base-sysctrl", "syscon";
> reg = <xyz>;
> #reset-cells = <1>;
> };
>
> something {
> ...
> resets = <&hbsys 0>;
> };
>
> something-else {
> ...
> resets = <&hbsys 1>;
> };
>
>
> Then if there is actually any meaningful functionality in the default
> extsys0 firmware preloaded on the FPGA setup then define a binding for
> "arm,corstone1000-an550-extsys0" to describe whatever that actually does. If
> a user chooses to create and load their own different firmware, they're
> going to need their own binding and driver for whatever *that* firmware
> does.
>
> FWIW, driver-wise the mapping to the reset API seems straightforward -
> .assert hits RST_REQ, .deassert clears CPUWAIT (.status is possibly a
> combination of CPUWAIT and RST_ACK?)
We are happy to follow what Robin recommended.
This can be summarized in two parts:
Part 1: Writing an SSE-710 reset controller driver
An SSE-710 reset controller driver that switches on/off the external system.
The driver will be helpful for products using SSE-710. So whoever licenses
Corstone-1000 or SSE-710 will find the reset controller driver helpful.
They can use it with their implementation of the external system.
Note: It's likely that the external systems the end user will be using in
their products will be different from the Corstone-1000 external system
given as an example. Differences in the memory configuration, subsystem
involved, boot roms configurations, ...
These differences mean that the end user will need to write their own driver
which might or might not be a remoteproc driver (e.g: Bluetooth, audio, ...).
Part 2: Corstone-1000 remoteproc driver
Corstone-1000 HW is being upgraded to support memory sharing between the
Cortex-A35 (Linux) and the external system (Cortex-M3).
Once the HW is ready, we can write a Corstone-1000 remoteproc driver able
to reload the external system firmware and doing communication with the MHUs.
This remoteproc driver can use the reset subsystem APIs to call the SSE-710
reset controller driver to switch on/off the external system (already
developed in part 1).
Impact on the current patchset:
- The current remoteproc patchset will be paused until the HW is upgraded.
- In CY25Q1, I'll send to the mailing list the SSE-710 reset controller bindings
and a driver under the Reset Controller subsystem.
Thank you for your support and expertise.
Cheers,
Abdellatif
^ permalink raw reply [flat|nested] 59+ messages in thread
end of thread, other threads:[~2024-10-09 9:46 UTC | newest]
Thread overview: 59+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-01 16:42 [PATCH 0/3] remoteproc: introduce Arm remoteproc support abdellatif.elkhlifi
2024-03-01 16:42 ` [PATCH 1/3] remoteproc: Add Arm remoteproc driver abdellatif.elkhlifi
2024-03-04 18:30 ` Rob Herring
2024-03-04 18:42 ` Mathieu Poirier
2024-03-07 19:40 ` Abdellatif El Khlifi
2024-03-08 16:44 ` Mathieu Poirier
2024-03-11 11:44 ` Abdellatif El Khlifi
2024-03-12 16:29 ` Mathieu Poirier
2024-03-12 17:32 ` Abdellatif El Khlifi
2024-03-13 16:25 ` Mathieu Poirier
2024-03-13 17:17 ` Abdellatif El Khlifi
2024-03-14 14:52 ` Mathieu Poirier
2024-03-14 14:59 ` Sudeep Holla
2024-03-14 15:16 ` Abdellatif El Khlifi
2024-03-14 15:24 ` Sudeep Holla
2024-03-14 16:29 ` Mathieu Poirier
2024-03-25 17:13 ` Abdellatif El Khlifi
2024-03-26 14:20 ` Mathieu Poirier
2024-03-26 17:14 ` Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 0/5] remoteproc: arm64: Introduce remoteproc support for Corstone-1000 External Systems Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors Abdellatif El Khlifi
2024-08-22 18:25 ` Rob Herring (Arm)
2024-08-23 6:23 ` Krzysztof Kozlowski
2024-09-19 9:35 ` Abdellatif El Khlifi
2024-09-19 10:04 ` Krzysztof Kozlowski
2024-09-19 14:57 ` Abdellatif El Khlifi
2024-09-20 12:42 ` Krzysztof Kozlowski
2024-09-20 14:19 ` Abdellatif El Khlifi
2024-09-20 14:56 ` Krzysztof Kozlowski
2024-09-20 16:38 ` Abdellatif El Khlifi
2024-09-21 18:20 ` Krzysztof Kozlowski
2024-09-23 11:49 ` Abdellatif El Khlifi
2024-09-23 15:29 ` Krzysztof Kozlowski
2024-09-23 17:19 ` Abdellatif El Khlifi
2024-09-27 7:57 ` Krzysztof Kozlowski
2024-09-22 18:58 ` Krzysztof Kozlowski
2024-09-27 17:54 ` Robin Murphy
2024-10-09 9:46 ` Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 2/5] dt-bindings: arm: sse710: Add Host Base System Control Abdellatif El Khlifi
2024-08-23 6:25 ` Krzysztof Kozlowski
2024-08-22 17:09 ` [PATCH v2 3/5] arm64: dts: corstone1000: Add MHU nodes used by the External System Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 4/5] arm64: dts: corstone1000: Add External System support Abdellatif El Khlifi
2024-08-22 17:09 ` [PATCH v2 5/5] remoteproc: arm64: corstone1000: Add the External Systems driver Abdellatif El Khlifi
2024-09-18 15:40 ` Abdellatif El Khlifi
2024-09-19 8:37 ` Mathieu Poirier
2024-03-01 16:42 ` [PATCH 2/3] arm64: dts: Add corstone1000 external system device node abdellatif.elkhlifi
2024-03-01 19:27 ` Krzysztof Kozlowski
2024-03-08 12:21 ` Sudeep Holla
2024-03-08 14:25 ` Abdellatif El Khlifi
2024-03-01 16:42 ` [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc abdellatif.elkhlifi
2024-03-01 19:30 ` Krzysztof Kozlowski
2024-03-08 12:29 ` Sudeep Holla
2024-03-08 13:54 ` Abdellatif El Khlifi
2024-03-13 19:59 ` Robin Murphy
2024-03-14 13:49 ` Abdellatif El Khlifi
2024-03-14 13:56 ` Krzysztof Kozlowski
2024-03-14 15:20 ` Abdellatif El Khlifi
2024-03-14 15:19 ` Sudeep Holla
2024-03-15 14:22 ` Abdellatif El Khlifi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).