[PATCH v2 0/5] K3 R5F & DSP IPC-only mode support

[PATCH v2 0/5] K3 R5F & DSP IPC-only mode support

[Suman Anna]

Hi All,

The following is a revised version of the series that adds the IPC-only
mode support for the TI K3 R5F and DSP (C66x and C71x) remoteprocs
covering AM65x, J721E, J7200 and AM64x SoCs. Patches are on top of
5.14-rc1 (the other dependent patches from v1 made it into 5.14-rc1).

Please see the v1 cover-letter [1] for the design details of the
'IPC-only' mode functionality.

The following are the main changes from v1, please see the individual
patches for the exact deltas:
 - The first patch in v1 "remoteproc: Introduce rproc_detach_device()
   wrapper" is dropped
 - Removed the addition of the rproc state flag 'detach_on_shutdown'
   and the 'ipc-only' state flag in each of the remoteproc drivers
 - IPC-only mode and remoteproc mode are supported by registering only
   the appropriate rproc ops.

The following is a summary of patches in v2:
 - Patch 1 enhances the remoteproc core to restrict stop on early-booted
   remoteprocs. 
 - Patches 2 and 4 refactor the mailbox request code out of start
   in the K3 R5F and DSP remoteproc drivers for reuse in the new attach
   callbacks.
 - Patch 3 adds the IPC-only mode support for R5F.
 - Patch 5 adds the IPC-only mode support for both K3 C66x and C71x
   DSPs.

I have re-verified the different combinations on J721E, J7200 and AM65x
SoCs. AM64x currently lacks early-boot support, but the logic is ready
for Single-CPU and Split modes that are specific to AM64x SoCs. 

regards
Suman

[1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20210522000309.26134-1-s-anna@ti.com/

Suman Anna (5):
  remoteproc: Add support for detach-only during shutdown
  remoteproc: k3-r5: Refactor mbox request code in start
  remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs
  remoteproc: k3-dsp: Refactor mbox request code in start
  remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

 drivers/remoteproc/remoteproc_cdev.c      |   7 +
 drivers/remoteproc/remoteproc_core.c      |   5 +-
 drivers/remoteproc/remoteproc_sysfs.c     |   6 +
 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 197 ++++++++++++----
 drivers/remoteproc/ti_k3_r5_remoteproc.c  | 265 +++++++++++++++++++---
 5 files changed, 407 insertions(+), 73 deletions(-)

-- 
2.32.0


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

[PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Suman Anna]

The remoteproc core has support for both stopping and detaching a
remote processor that was attached to previously, through both the
remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
unconditionally only uses the stop functionality at present. This
may not be the default desired functionality for all the remoteproc
platform drivers.

Enhance the remoteproc core logic to key off the presence of the
.stop() ops and allow the individual remoteproc drivers to continue
to use the standard rproc_add() and rproc_del() API. This allows
the remoteproc drivers to only do detach if supported when the driver
is uninstalled, and the remote processor continues to run undisturbed
even after the driver removal.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
v2: Addressed various review comments from v1
 - Reworked the logic to not use remoteproc detach_on_shutdown and
   rely only on rproc callback ops
 - Updated the last para of the patch description
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/

 drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
 drivers/remoteproc/remoteproc_core.c  | 5 ++++-
 drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 4ad98b0b8caa..16c932beed88 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
 		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
+		if (rproc->state == RPROC_ATTACHED &&
+		    !rproc->ops->stop) {
+			dev_err(&rproc->dev,
+				"stop not supported for this rproc, use detach\n");
+			return -EINVAL;
+		}
+
 		rproc_shutdown(rproc);
 	} else if (!strncmp(cmd, "detach", len)) {
 		if (rproc->state != RPROC_ATTACHED)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7de5905d276a..ab9e52180b04 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
 	if (!atomic_dec_and_test(&rproc->power))
 		goto out;
 
-	ret = rproc_stop(rproc, false);
+	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
+		ret = __rproc_detach(rproc);
+	else
+		ret = rproc_stop(rproc, false);
 	if (ret) {
 		atomic_inc(&rproc->power);
 		goto out;
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index ea8b89f97d7b..133e766f38d4 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
 		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
+		if (rproc->state == RPROC_ATTACHED &&
+		    !rproc->ops->stop) {
+			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
+			return -EINVAL;
+		}
+
 		rproc_shutdown(rproc);
 	} else if (sysfs_streq(buf, "detach")) {
 		if (rproc->state != RPROC_ATTACHED)
-- 
2.32.0


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

[PATCH v2 2/5] remoteproc: k3-r5: Refactor mbox request code in start

[Suman Anna]

Refactor out the mailbox request and associated ping logic code
from k3_r5_rproc_start() function into its own separate function
so that it can be re-used in the soon to be added .attach() ops
callback.

Signed-off-by: Suman Anna <s-anna@ti.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
v2: No code changes, picked up Reviewed-by tags
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-4-s-anna@ti.com/

 drivers/remoteproc/ti_k3_r5_remoteproc.c | 66 ++++++++++++++----------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 71615210df3e..03f930758b2d 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -376,6 +376,44 @@ static inline int k3_r5_core_run(struct k3_r5_core *core)
 				       0, PROC_BOOT_CTRL_FLAG_R5_CORE_HALT);
 }
 
+static int k3_r5_rproc_request_mbox(struct rproc *rproc)
+{
+	struct k3_r5_rproc *kproc = rproc->priv;
+	struct mbox_client *client = &kproc->client;
+	struct device *dev = kproc->dev;
+	int ret;
+
+	client->dev = dev;
+	client->tx_done = NULL;
+	client->rx_callback = k3_r5_rproc_mbox_callback;
+	client->tx_block = false;
+	client->knows_txdone = false;
+
+	kproc->mbox = mbox_request_channel(client, 0);
+	if (IS_ERR(kproc->mbox)) {
+		ret = -EBUSY;
+		dev_err(dev, "mbox_request_channel failed: %ld\n",
+			PTR_ERR(kproc->mbox));
+		return ret;
+	}
+
+	/*
+	 * Ping the remote processor, this is only for sanity-sake for now;
+	 * there is no functional effect whatsoever.
+	 *
+	 * Note that the reply will _not_ arrive immediately: this message
+	 * will wait in the mailbox fifo until the remote processor is booted.
+	 */
+	ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_ECHO_REQUEST);
+	if (ret < 0) {
+		dev_err(dev, "mbox_send_message failed: %d\n", ret);
+		mbox_free_channel(kproc->mbox);
+		return ret;
+	}
+
+	return 0;
+}
+
 /*
  * The R5F cores have controls for both a reset and a halt/run. The code
  * execution from DDR requires the initial boot-strapping code to be run
@@ -495,38 +533,14 @@ static int k3_r5_rproc_start(struct rproc *rproc)
 {
 	struct k3_r5_rproc *kproc = rproc->priv;
 	struct k3_r5_cluster *cluster = kproc->cluster;
-	struct mbox_client *client = &kproc->client;
 	struct device *dev = kproc->dev;
 	struct k3_r5_core *core;
 	u32 boot_addr;
 	int ret;
 
-	client->dev = dev;
-	client->tx_done = NULL;
-	client->rx_callback = k3_r5_rproc_mbox_callback;
-	client->tx_block = false;
-	client->knows_txdone = false;
-
-	kproc->mbox = mbox_request_channel(client, 0);
-	if (IS_ERR(kproc->mbox)) {
-		ret = -EBUSY;
-		dev_err(dev, "mbox_request_channel failed: %ld\n",
-			PTR_ERR(kproc->mbox));
+	ret = k3_r5_rproc_request_mbox(rproc);
+	if (ret)
 		return ret;
-	}
-
-	/*
-	 * Ping the remote processor, this is only for sanity-sake for now;
-	 * there is no functional effect whatsoever.
-	 *
-	 * Note that the reply will _not_ arrive immediately: this message
-	 * will wait in the mailbox fifo until the remote processor is booted.
-	 */
-	ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_ECHO_REQUEST);
-	if (ret < 0) {
-		dev_err(dev, "mbox_send_message failed: %d\n", ret);
-		goto put_mbox;
-	}
 
 	boot_addr = rproc->bootaddr;
 	/* TODO: add boot_addr sanity checking */
-- 
2.32.0


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

[PATCH v2 3/5] remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs

[Suman Anna]

Add support to the K3 R5F remoteproc driver to configure all the R5F
cores to be either in IPC-only mode or the traditional remoteproc mode.
The IPC-only mode expects that the remote processors are already booted
by the bootloader, and only performs the minimum steps required to
initialize and deinitialize the virtio IPC transports. The remoteproc
mode allows the kernel remoteproc driver to do the regular load and
boot and other device management operations for a R5F core.

The IPC-only mode for a R5F core is detected and configured at driver
probe time by querying the System Firmware for the R5F power and reset
state and/or status and making sure that the R5F core is indeed started
by the bootloaders, otherwise the device is configured for remoteproc
mode.

Support for IPC-only mode is achieved through .attach(), .detach() and
.get_loaded_rsc_table() callback ops and zeroing out the regular rproc
ops .prepare(), .unprepare(), .start() and .stop(). The resource table
follows a design-by-contract approach and is expected to be at the base
of the DDR firmware region reserved for each remoteproc, it is mostly
expected to contain only the virtio device and trace resource entries.

NOTE:
The driver cannot configure a R5F core for remoteproc mode by any
means without rebooting the kernel if that R5F core has been started
by a bootloader. This is the current desired behavior and can be
enhanced in the future if the feature is needed.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
v2: Addressed various review comments from v1
 - Reworked the logic to not use remoteproc detach_on_shutdown and
   local ipc-only state flags
 - Plugged in the required IPC-only ops dynamically with the regular
   remoteproc-mode ops zeroed out
 - Dropped all the unneeded error checks in start, stop, prepare, 
   unprepare, attach and detach callbacks
 - Callback function descriptions updated to reflect the mode they
   apply to
 - Switched to dev_info for the mode information traces from dev_err
 - Revised the last 2 paras of the patch description
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-5-s-anna@ti.com/

 drivers/remoteproc/ti_k3_r5_remoteproc.c | 199 ++++++++++++++++++++++-
 1 file changed, 196 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 03f930758b2d..12658368aed6 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -428,6 +428,7 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
  * private to each core. Only Core0 needs to be unhalted for running the
  * cluster in this mode. The function uses the same reset logic as LockStep
  * mode for this (though the behavior is agnostic of the reset release order).
+ * This callback is invoked only in remoteproc mode.
  */
 static int k3_r5_rproc_prepare(struct rproc *rproc)
 {
@@ -493,7 +494,8 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
  * both cores. The access is made possible only with releasing the resets for
  * both cores, but with only Core0 unhalted. This function re-uses the same
  * reset assert logic as LockStep mode for this mode (though the behavior is
- * agnostic of the reset assert order).
+ * agnostic of the reset assert order). This callback is invoked only in
+ * remoteproc mode.
  */
 static int k3_r5_rproc_unprepare(struct rproc *rproc)
 {
@@ -527,7 +529,8 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc)
  *
  * The Single-CPU mode on applicable SoCs (eg: AM64x) only uses Core0 to execute
  * code, so only Core0 needs to be unhalted. The function uses the same logic
- * flow as Split-mode for this.
+ * flow as Split-mode for this. This callback is invoked only in remoteproc
+ * mode.
  */
 static int k3_r5_rproc_start(struct rproc *rproc)
 {
@@ -598,7 +601,8 @@ static int k3_r5_rproc_start(struct rproc *rproc)
  * be done here, but is preferred to be done in the .unprepare() ops - this
  * maintains the symmetric behavior between the .start(), .stop(), .prepare()
  * and .unprepare() ops, and also balances them well between sysfs 'state'
- * flow and device bind/unbind or module removal.
+ * flow and device bind/unbind or module removal. This callback is invoked
+ * only in remoteproc mode.
  */
 static int k3_r5_rproc_stop(struct rproc *rproc)
 {
@@ -635,6 +639,78 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
 	return ret;
 }
 
+/*
+ * Attach to a running R5F remote processor (IPC-only mode)
+ *
+ * The R5F attach callback only needs to request the mailbox, the remote
+ * processor is already booted, so there is no need to issue any TI-SCI
+ * commands to boot the R5F cores in IPC-only mode. This callback is invoked
+ * only in IPC-only mode.
+ */
+static int k3_r5_rproc_attach(struct rproc *rproc)
+{
+	struct k3_r5_rproc *kproc = rproc->priv;
+	struct device *dev = kproc->dev;
+	int ret;
+
+	ret = k3_r5_rproc_request_mbox(rproc);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "R5F core initialized in IPC-only mode\n");
+	return 0;
+}
+
+/*
+ * Detach from a running R5F remote processor (IPC-only mode)
+ *
+ * The R5F detach callback performs the opposite operation to attach callback
+ * and only needs to release the mailbox, the R5F cores are not stopped and
+ * will be left in booted state in IPC-only mode. This callback is invoked
+ * only in IPC-only mode.
+ */
+static int k3_r5_rproc_detach(struct rproc *rproc)
+{
+	struct k3_r5_rproc *kproc = rproc->priv;
+	struct device *dev = kproc->dev;
+
+	mbox_free_channel(kproc->mbox);
+	dev_info(dev, "R5F core deinitialized in IPC-only mode\n");
+	return 0;
+}
+
+/*
+ * This function implements the .get_loaded_rsc_table() callback and is used
+ * to provide the resource table for the booted R5F in IPC-only mode. The K3 R5F
+ * firmwares follow a design-by-contract approach and are expected to have the
+ * resource table at the base of the DDR region reserved for firmware usage.
+ * This provides flexibility for the remote processor to be booted by different
+ * bootloaders that may or may not have the ability to publish the resource table
+ * address and size through a DT property. This callback is invoked only in
+ * IPC-only mode.
+ */
+static struct resource_table *k3_r5_get_loaded_rsc_table(struct rproc *rproc,
+							 size_t *rsc_table_sz)
+{
+	struct k3_r5_rproc *kproc = rproc->priv;
+	struct device *dev = kproc->dev;
+
+	if (!kproc->rmem[0].cpu_addr) {
+		dev_err(dev, "memory-region #1 does not exist, loaded rsc table can't be found");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/*
+	 * NOTE: The resource table size is currently hard-coded to a maximum
+	 * of 256 bytes. The most common resource table usage for K3 firmwares
+	 * is to only have the vdev resource entry and an optional trace entry.
+	 * The exact size could be computed based on resource table address, but
+	 * the hard-coded value suffices to support the IPC-only mode.
+	 */
+	*rsc_table_sz = 256;
+	return (struct resource_table *)kproc->rmem[0].cpu_addr;
+}
+
 /*
  * Internal Memory translation helper
  *
@@ -1014,6 +1090,116 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
 	}
 }
 
+/*
+ * This function checks and configures a R5F core for IPC-only or remoteproc
+ * mode. The driver is configured to be in IPC-only mode for a R5F core when
+ * the core has been loaded and started by a bootloader. The IPC-only mode is
+ * detected by querying the System Firmware for reset, power on and halt status
+ * and ensuring that the core is running. Any incomplete steps at bootloader
+ * are validated and errored out.
+ *
+ * In IPC-only mode, the driver state flags for ATCM, BTCM and LOCZRAMA settings
+ * and cluster mode parsed originally from kernel DT are updated to reflect the
+ * actual values configured by bootloader. The driver internal device memory
+ * addresses for TCMs are also updated.
+ */
+static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
+{
+	struct k3_r5_cluster *cluster = kproc->cluster;
+	struct k3_r5_core *core = kproc->core;
+	struct device *cdev = core->dev;
+	bool r_state = false, c_state = false;
+	u32 ctrl = 0, cfg = 0, stat = 0, halted = 0;
+	u64 boot_vec = 0;
+	u32 atcm_enable, btcm_enable, loczrama;
+	struct k3_r5_core *core0;
+	enum cluster_mode mode;
+	int ret;
+
+	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
+
+	ret = core->ti_sci->ops.dev_ops.is_on(core->ti_sci, core->ti_sci_id,
+					      &r_state, &c_state);
+	if (ret) {
+		dev_err(cdev, "failed to get initial state, mode cannot be determined, ret = %d\n",
+			ret);
+		return ret;
+	}
+	if (r_state != c_state) {
+		dev_warn(cdev, "R5F core may have been powered on by a different host, programmed state (%d) != actual state (%d)\n",
+			 r_state, c_state);
+	}
+
+	ret = reset_control_status(core->reset);
+	if (ret < 0) {
+		dev_err(cdev, "failed to get initial local reset status, ret = %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl,
+				     &stat);
+	if (ret < 0) {
+		dev_err(cdev, "failed to get initial processor status, ret = %d\n",
+			ret);
+		return ret;
+	}
+	atcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_ATCM_EN ?  1 : 0;
+	btcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_BTCM_EN ?  1 : 0;
+	loczrama = cfg & PROC_BOOT_CFG_FLAG_R5_TCM_RSTBASE ?  1 : 0;
+	if (cluster->soc_data->single_cpu_mode) {
+		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
+				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+	} else {
+		mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
+				CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;
+	}
+	halted = ctrl & PROC_BOOT_CTRL_FLAG_R5_CORE_HALT;
+
+	/*
+	 * IPC-only mode detection requires both local and module resets to
+	 * be deasserted and R5F core to be unhalted. Local reset status is
+	 * irrelevant if module reset is asserted (POR value has local reset
+	 * deasserted), and is deemed as remoteproc mode
+	 */
+	if (c_state && !ret && !halted) {
+		dev_info(cdev, "configured R5F for IPC-only mode\n");
+		kproc->rproc->state = RPROC_DETACHED;
+		ret = 1;
+		/* override rproc ops with only required IPC-only mode ops */
+		kproc->rproc->ops->prepare = NULL;
+		kproc->rproc->ops->unprepare = NULL;
+		kproc->rproc->ops->start = NULL;
+		kproc->rproc->ops->stop = NULL;
+		kproc->rproc->ops->attach = k3_r5_rproc_attach;
+		kproc->rproc->ops->detach = k3_r5_rproc_detach;
+		kproc->rproc->ops->get_loaded_rsc_table =
+						k3_r5_get_loaded_rsc_table;
+	} else if (!c_state) {
+		dev_info(cdev, "configured R5F for remoteproc mode\n");
+		ret = 0;
+	} else {
+		dev_err(cdev, "mismatched mode: local_reset = %s, module_reset = %s, core_state = %s\n",
+			!ret ? "deasserted" : "asserted",
+			c_state ? "deasserted" : "asserted",
+			halted ? "halted" : "unhalted");
+		ret = -EINVAL;
+	}
+
+	/* fixup TCMs, cluster & core flags to actual values in IPC-only mode */
+	if (ret > 0) {
+		if (core == core0)
+			cluster->mode = mode;
+		core->atcm_enable = atcm_enable;
+		core->btcm_enable = btcm_enable;
+		core->loczrama = loczrama;
+		core->mem[0].dev_addr = loczrama ? 0 : K3_R5_TCM_DEV_ADDR;
+		core->mem[1].dev_addr = loczrama ? K3_R5_TCM_DEV_ADDR : 0;
+	}
+
+	return ret;
+}
+
 static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 {
 	struct k3_r5_cluster *cluster = platform_get_drvdata(pdev);
@@ -1054,6 +1240,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 		kproc->rproc = rproc;
 		core->rproc = rproc;
 
+		ret = k3_r5_rproc_configure_mode(kproc);
+		if (ret < 0)
+			goto err_config;
+		if (ret)
+			goto init_rmem;
+
 		ret = k3_r5_rproc_configure(kproc);
 		if (ret) {
 			dev_err(dev, "initial configure failed, ret = %d\n",
@@ -1061,6 +1253,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			goto err_config;
 		}
 
+init_rmem:
 		k3_r5_adjust_tcm_sizes(kproc);
 
 		ret = k3_r5_reserved_mem_init(kproc);
-- 
2.32.0


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

[PATCH v2 4/5] remoteproc: k3-dsp: Refactor mbox request code in start

[Suman Anna]

Refactor out the mailbox request and associated ping logic code
from k3_dsp_rproc_start() function into its own separate function
so that it can be re-used in the soon to be added .attach() ops
callback.

Signed-off-by: Suman Anna <s-anna@ti.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
v2: No code changes, picked up Reviewed-by tags
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-6-s-anna@ti.com/

 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 65 ++++++++++++++---------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
index fd4eb67a6681..faf60a274e8d 100644
--- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
@@ -216,6 +216,43 @@ static int k3_dsp_rproc_release(struct k3_dsp_rproc *kproc)
 	return ret;
 }
 
+static int k3_dsp_rproc_request_mbox(struct rproc *rproc)
+{
+	struct k3_dsp_rproc *kproc = rproc->priv;
+	struct mbox_client *client = &kproc->client;
+	struct device *dev = kproc->dev;
+	int ret;
+
+	client->dev = dev;
+	client->tx_done = NULL;
+	client->rx_callback = k3_dsp_rproc_mbox_callback;
+	client->tx_block = false;
+	client->knows_txdone = false;
+
+	kproc->mbox = mbox_request_channel(client, 0);
+	if (IS_ERR(kproc->mbox)) {
+		ret = -EBUSY;
+		dev_err(dev, "mbox_request_channel failed: %ld\n",
+			PTR_ERR(kproc->mbox));
+		return ret;
+	}
+
+	/*
+	 * Ping the remote processor, this is only for sanity-sake for now;
+	 * there is no functional effect whatsoever.
+	 *
+	 * Note that the reply will _not_ arrive immediately: this message
+	 * will wait in the mailbox fifo until the remote processor is booted.
+	 */
+	ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_ECHO_REQUEST);
+	if (ret < 0) {
+		dev_err(dev, "mbox_send_message failed: %d\n", ret);
+		mbox_free_channel(kproc->mbox);
+		return ret;
+	}
+
+	return 0;
+}
 /*
  * The C66x DSP cores have a local reset that affects only the CPU, and a
  * generic module reset that powers on the device and allows the DSP internal
@@ -273,37 +310,13 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
 static int k3_dsp_rproc_start(struct rproc *rproc)
 {
 	struct k3_dsp_rproc *kproc = rproc->priv;
-	struct mbox_client *client = &kproc->client;
 	struct device *dev = kproc->dev;
 	u32 boot_addr;
 	int ret;
 
-	client->dev = dev;
-	client->tx_done = NULL;
-	client->rx_callback = k3_dsp_rproc_mbox_callback;
-	client->tx_block = false;
-	client->knows_txdone = false;
-
-	kproc->mbox = mbox_request_channel(client, 0);
-	if (IS_ERR(kproc->mbox)) {
-		ret = -EBUSY;
-		dev_err(dev, "mbox_request_channel failed: %ld\n",
-			PTR_ERR(kproc->mbox));
+	ret = k3_dsp_rproc_request_mbox(rproc);
+	if (ret)
 		return ret;
-	}
-
-	/*
-	 * Ping the remote processor, this is only for sanity-sake for now;
-	 * there is no functional effect whatsoever.
-	 *
-	 * Note that the reply will _not_ arrive immediately: this message
-	 * will wait in the mailbox fifo until the remote processor is booted.
-	 */
-	ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_ECHO_REQUEST);
-	if (ret < 0) {
-		dev_err(dev, "mbox_send_message failed: %d\n", ret);
-		goto put_mbox;
-	}
 
 	boot_addr = rproc->bootaddr;
 	if (boot_addr & (kproc->data->boot_align_addr - 1)) {
-- 
2.32.0


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

[PATCH v2 5/5] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

[Suman Anna]

Add support to the K3 DSP remoteproc driver to configure all the C66x
and C71x cores on J721E SoCs to be either in IPC-only mode or the
traditional remoteproc mode. The IPC-only mode expects that the remote
processors are already booted by the bootloader, and only perform the
minimum steps required to initialize and deinitialize the virtio IPC
transports. The remoteproc mode allows the kernel remoteproc driver to
do the regular load and boot and other device management operations for
a DSP.

The IPC-only mode for a DSP is detected and configured at driver probe
time by querying the System Firmware for the DSP power and reset state
and/or status and making sure that the DSP is indeed started by the
bootloaders, otherwise the device is configured for remoteproc mode.

Support for IPC-only mode is achieved through .attach(), .detach() and
.get_loaded_rsc_table() callback ops and zeroing out the regular rproc
ops .prepare(), .unprepare(), .start() and .stop(). The resource table
follows a design-by-contract approach and is expected to be at the base
of the DDR firmware region reserved for each remoteproc, it is mostly
expected to contain only the virtio device and trace resource entries.

NOTE:
The driver cannot configure a DSP core for remoteproc mode by any
means without rebooting the kernel if that DSP core has been started
by a bootloader.  This is the current desired behavior and can be
enhanced in the future if the feature is needed.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
v2: Addressed various review comments from v1
 - Reworked the logic to not use remoteproc detach_on_shutdown and
   local ipc-only state flags
 - Plugged in the required IPC-only ops dynamically with the regular
   remoteproc-mode ops zeroed out
 - Dropped all the unneeded error checks in start, stop, prepare, 
   unprepare, attach and detach callbacks
 - Callback function descriptions updated to reflect the mode they
   apply to
 - Dropped unused r_state variable in probe
 - Switched to dev_info for the mode information traces from dev_err
 - Revised the last 2 paras of the patch description
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-7-s-anna@ti.com/

 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 132 +++++++++++++++++++---
 1 file changed, 115 insertions(+), 17 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
index faf60a274e8d..6eaecf02aee5 100644
--- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
@@ -260,7 +260,8 @@ static int k3_dsp_rproc_request_mbox(struct rproc *rproc)
  * used to release the global reset on C66x DSPs to allow loading into the DSP
  * internal RAMs. The .prepare() ops is invoked by remoteproc core before any
  * firmware loading, and is followed by the .start() ops after loading to
- * actually let the C66x DSP cores run.
+ * actually let the C66x DSP cores run. This callback is invoked only in
+ * remoteproc mode.
  */
 static int k3_dsp_rproc_prepare(struct rproc *rproc)
 {
@@ -284,7 +285,7 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
  * powering down the C66x DSP cores. The cores themselves are only halted in the
  * .stop() callback through the local reset, and the .unprepare() ops is invoked
  * by the remoteproc core after the remoteproc is stopped to balance the global
- * reset.
+ * reset. This callback is invoked only in remoteproc mode.
  */
 static int k3_dsp_rproc_unprepare(struct rproc *rproc)
 {
@@ -305,7 +306,7 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
  *
  * This function will be invoked only after the firmware for this rproc
  * was loaded, parsed successfully, and all of its resource requirements
- * were met.
+ * were met. This callback is invoked only in remoteproc mode.
  */
 static int k3_dsp_rproc_start(struct rproc *rproc)
 {
@@ -346,7 +347,7 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
  * Stop the DSP remote processor.
  *
  * This function puts the DSP processor into reset, and finishes processing
- * of any pending messages.
+ * of any pending messages. This callback is invoked only in remoteproc mode.
  */
 static int k3_dsp_rproc_stop(struct rproc *rproc)
 {
@@ -359,6 +360,78 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
 	return 0;
 }
 
+/*
+ * Attach to a running DSP remote processor (IPC-only mode)
+ *
+ * This rproc attach callback only needs to request the mailbox, the remote
+ * processor is already booted, so there is no need to issue any TI-SCI
+ * commands to boot the DSP core. This callback is invoked only in IPC-only
+ * mode.
+ */
+static int k3_dsp_rproc_attach(struct rproc *rproc)
+{
+	struct k3_dsp_rproc *kproc = rproc->priv;
+	struct device *dev = kproc->dev;
+	int ret;
+
+	ret = k3_dsp_rproc_request_mbox(rproc);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "DSP initialized in IPC-only mode\n");
+	return 0;
+}
+
+/*
+ * Detach from a running DSP remote processor (IPC-only mode)
+ *
+ * This rproc detach callback performs the opposite operation to attach callback
+ * and only needs to release the mailbox, the DSP core is not stopped and will
+ * be left to continue to run its booted firmware. This callback is invoked only
+ * in IPC-only mode.
+ */
+static int k3_dsp_rproc_detach(struct rproc *rproc)
+{
+	struct k3_dsp_rproc *kproc = rproc->priv;
+	struct device *dev = kproc->dev;
+
+	mbox_free_channel(kproc->mbox);
+	dev_info(dev, "DSP deinitialized in IPC-only mode\n");
+	return 0;
+}
+
+/*
+ * This function implements the .get_loaded_rsc_table() callback and is used
+ * to provide the resource table for a booted DSP in IPC-only mode. The K3 DSP
+ * firmwares follow a design-by-contract approach and are expected to have the
+ * resource table at the base of the DDR region reserved for firmware usage.
+ * This provides flexibility for the remote processor to be booted by different
+ * bootloaders that may or may not have the ability to publish the resource table
+ * address and size through a DT property. This callback is invoked only in
+ * IPC-only mode.
+ */
+static struct resource_table *k3_dsp_get_loaded_rsc_table(struct rproc *rproc,
+							  size_t *rsc_table_sz)
+{
+	struct k3_dsp_rproc *kproc = rproc->priv;
+	struct device *dev = kproc->dev;
+
+	if (!kproc->rmem[0].cpu_addr) {
+		dev_err(dev, "memory-region #1 does not exist, loaded rsc table can't be found");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/*
+	 * NOTE: The resource table size is currently hard-coded to a maximum
+	 * of 256 bytes. The most common resource table usage for K3 firmwares
+	 * is to only have the vdev resource entry and an optional trace entry.
+	 * The exact size could be computed based on resource table address, but
+	 * the hard-coded value suffices to support the IPC-only mode.
+	 */
+	*rsc_table_sz = 256;
+	return (struct resource_table *)kproc->rmem[0].cpu_addr;
+}
+
 /*
  * Custom function to translate a DSP device address (internal RAMs only) to a
  * kernel virtual address.  The DSPs can access their RAMs at either an internal
@@ -605,6 +678,7 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
 	struct k3_dsp_rproc *kproc;
 	struct rproc *rproc;
 	const char *fw_name;
+	bool p_state = false;
 	int ret = 0;
 	int ret1;
 
@@ -683,19 +757,43 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
 		goto release_tsp;
 	}
 
-	/*
-	 * ensure the DSP local reset is asserted to ensure the DSP doesn't
-	 * execute bogus code in .prepare() when the module reset is released.
-	 */
-	if (data->uses_lreset) {
-		ret = reset_control_status(kproc->reset);
-		if (ret < 0) {
-			dev_err(dev, "failed to get reset status, status = %d\n",
-				ret);
-			goto release_mem;
-		} else if (ret == 0) {
-			dev_warn(dev, "local reset is deasserted for device\n");
-			k3_dsp_rproc_reset(kproc);
+	ret = kproc->ti_sci->ops.dev_ops.is_on(kproc->ti_sci, kproc->ti_sci_id,
+					       NULL, &p_state);
+	if (ret) {
+		dev_err(dev, "failed to get initial state, mode cannot be determined, ret = %d\n",
+			ret);
+		goto release_mem;
+	}
+
+	/* configure J721E devices for either remoteproc or IPC-only mode */
+	if (p_state) {
+		dev_info(dev, "configured DSP for IPC-only mode\n");
+		rproc->state = RPROC_DETACHED;
+		/* override rproc ops with only required IPC-only mode ops */
+		rproc->ops->prepare = NULL;
+		rproc->ops->unprepare = NULL;
+		rproc->ops->start = NULL;
+		rproc->ops->stop = NULL;
+		rproc->ops->attach = k3_dsp_rproc_attach;
+		rproc->ops->detach = k3_dsp_rproc_detach;
+		rproc->ops->get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table;
+	} else {
+		dev_info(dev, "configured DSP for remoteproc mode\n");
+		/*
+		 * ensure the DSP local reset is asserted to ensure the DSP
+		 * doesn't execute bogus code in .prepare() when the module
+		 * reset is released.
+		 */
+		if (data->uses_lreset) {
+			ret = reset_control_status(kproc->reset);
+			if (ret < 0) {
+				dev_err(dev, "failed to get reset status, status = %d\n",
+					ret);
+				goto release_mem;
+			} else if (ret == 0) {
+				dev_warn(dev, "local reset is deasserted for device\n");
+				k3_dsp_rproc_reset(kproc);
+			}
 		}
 	}
 
-- 
2.32.0


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

Re: [PATCH v2 0/5] K3 R5F & DSP IPC-only mode support

[Mathieu Poirier]

Hi Suman,

I have added your patchset to my review list.  Unfortunately due to an
impressive backlog and upcoming vacation won't be able provide
feedback for a few weeks.

Thanks,
Mathieu

On Fri, 23 Jul 2021 at 16:03, Suman Anna <s-anna@ti.com> wrote:
>
> Hi All,
>
> The following is a revised version of the series that adds the IPC-only
> mode support for the TI K3 R5F and DSP (C66x and C71x) remoteprocs
> covering AM65x, J721E, J7200 and AM64x SoCs. Patches are on top of
> 5.14-rc1 (the other dependent patches from v1 made it into 5.14-rc1).
>
> Please see the v1 cover-letter [1] for the design details of the
> 'IPC-only' mode functionality.
>
> The following are the main changes from v1, please see the individual
> patches for the exact deltas:
>  - The first patch in v1 "remoteproc: Introduce rproc_detach_device()
>    wrapper" is dropped
>  - Removed the addition of the rproc state flag 'detach_on_shutdown'
>    and the 'ipc-only' state flag in each of the remoteproc drivers
>  - IPC-only mode and remoteproc mode are supported by registering only
>    the appropriate rproc ops.
>
> The following is a summary of patches in v2:
>  - Patch 1 enhances the remoteproc core to restrict stop on early-booted
>    remoteprocs.
>  - Patches 2 and 4 refactor the mailbox request code out of start
>    in the K3 R5F and DSP remoteproc drivers for reuse in the new attach
>    callbacks.
>  - Patch 3 adds the IPC-only mode support for R5F.
>  - Patch 5 adds the IPC-only mode support for both K3 C66x and C71x
>    DSPs.
>
> I have re-verified the different combinations on J721E, J7200 and AM65x
> SoCs. AM64x currently lacks early-boot support, but the logic is ready
> for Single-CPU and Split modes that are specific to AM64x SoCs.
>
> regards
> Suman
>
> [1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20210522000309.26134-1-s-anna@ti.com/
>
> Suman Anna (5):
>   remoteproc: Add support for detach-only during shutdown
>   remoteproc: k3-r5: Refactor mbox request code in start
>   remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs
>   remoteproc: k3-dsp: Refactor mbox request code in start
>   remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs
>
>  drivers/remoteproc/remoteproc_cdev.c      |   7 +
>  drivers/remoteproc/remoteproc_core.c      |   5 +-
>  drivers/remoteproc/remoteproc_sysfs.c     |   6 +
>  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 197 ++++++++++++----
>  drivers/remoteproc/ti_k3_r5_remoteproc.c  | 265 +++++++++++++++++++---
>  5 files changed, 407 insertions(+), 73 deletions(-)
>
> --
> 2.32.0
>

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

Re: [PATCH v2 3/5] remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs

[Mathieu Poirier]

On Fri, Jul 23, 2021 at 05:02:46PM -0500, Suman Anna wrote:
> Add support to the K3 R5F remoteproc driver to configure all the R5F
> cores to be either in IPC-only mode or the traditional remoteproc mode.
> The IPC-only mode expects that the remote processors are already booted
> by the bootloader, and only performs the minimum steps required to
> initialize and deinitialize the virtio IPC transports. The remoteproc
> mode allows the kernel remoteproc driver to do the regular load and
> boot and other device management operations for a R5F core.
> 
> The IPC-only mode for a R5F core is detected and configured at driver
> probe time by querying the System Firmware for the R5F power and reset
> state and/or status and making sure that the R5F core is indeed started
> by the bootloaders, otherwise the device is configured for remoteproc
> mode.
> 
> Support for IPC-only mode is achieved through .attach(), .detach() and
> .get_loaded_rsc_table() callback ops and zeroing out the regular rproc
> ops .prepare(), .unprepare(), .start() and .stop(). The resource table
> follows a design-by-contract approach and is expected to be at the base
> of the DDR firmware region reserved for each remoteproc, it is mostly
> expected to contain only the virtio device and trace resource entries.
> 
> NOTE:
> The driver cannot configure a R5F core for remoteproc mode by any
> means without rebooting the kernel if that R5F core has been started
> by a bootloader. This is the current desired behavior and can be
> enhanced in the future if the feature is needed.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: Addressed various review comments from v1
>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>    local ipc-only state flags
>  - Plugged in the required IPC-only ops dynamically with the regular
>    remoteproc-mode ops zeroed out
>  - Dropped all the unneeded error checks in start, stop, prepare, 
>    unprepare, attach and detach callbacks
>  - Callback function descriptions updated to reflect the mode they
>    apply to
>  - Switched to dev_info for the mode information traces from dev_err
>  - Revised the last 2 paras of the patch description
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-5-s-anna@ti.com/
> 
>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 199 ++++++++++++++++++++++-
>  1 file changed, 196 insertions(+), 3 deletions(-)
> 

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 03f930758b2d..12658368aed6 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -428,6 +428,7 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
>   * private to each core. Only Core0 needs to be unhalted for running the
>   * cluster in this mode. The function uses the same reset logic as LockStep
>   * mode for this (though the behavior is agnostic of the reset release order).
> + * This callback is invoked only in remoteproc mode.
>   */
>  static int k3_r5_rproc_prepare(struct rproc *rproc)
>  {
> @@ -493,7 +494,8 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
>   * both cores. The access is made possible only with releasing the resets for
>   * both cores, but with only Core0 unhalted. This function re-uses the same
>   * reset assert logic as LockStep mode for this mode (though the behavior is
> - * agnostic of the reset assert order).
> + * agnostic of the reset assert order). This callback is invoked only in
> + * remoteproc mode.
>   */
>  static int k3_r5_rproc_unprepare(struct rproc *rproc)
>  {
> @@ -527,7 +529,8 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc)
>   *
>   * The Single-CPU mode on applicable SoCs (eg: AM64x) only uses Core0 to execute
>   * code, so only Core0 needs to be unhalted. The function uses the same logic
> - * flow as Split-mode for this.
> + * flow as Split-mode for this. This callback is invoked only in remoteproc
> + * mode.
>   */
>  static int k3_r5_rproc_start(struct rproc *rproc)
>  {
> @@ -598,7 +601,8 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>   * be done here, but is preferred to be done in the .unprepare() ops - this
>   * maintains the symmetric behavior between the .start(), .stop(), .prepare()
>   * and .unprepare() ops, and also balances them well between sysfs 'state'
> - * flow and device bind/unbind or module removal.
> + * flow and device bind/unbind or module removal. This callback is invoked
> + * only in remoteproc mode.
>   */
>  static int k3_r5_rproc_stop(struct rproc *rproc)
>  {
> @@ -635,6 +639,78 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>  	return ret;
>  }
>  
> +/*
> + * Attach to a running R5F remote processor (IPC-only mode)
> + *
> + * The R5F attach callback only needs to request the mailbox, the remote
> + * processor is already booted, so there is no need to issue any TI-SCI
> + * commands to boot the R5F cores in IPC-only mode. This callback is invoked
> + * only in IPC-only mode.
> + */
> +static int k3_r5_rproc_attach(struct rproc *rproc)
> +{
> +	struct k3_r5_rproc *kproc = rproc->priv;
> +	struct device *dev = kproc->dev;
> +	int ret;
> +
> +	ret = k3_r5_rproc_request_mbox(rproc);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "R5F core initialized in IPC-only mode\n");
> +	return 0;
> +}
> +
> +/*
> + * Detach from a running R5F remote processor (IPC-only mode)
> + *
> + * The R5F detach callback performs the opposite operation to attach callback
> + * and only needs to release the mailbox, the R5F cores are not stopped and
> + * will be left in booted state in IPC-only mode. This callback is invoked
> + * only in IPC-only mode.
> + */
> +static int k3_r5_rproc_detach(struct rproc *rproc)
> +{
> +	struct k3_r5_rproc *kproc = rproc->priv;
> +	struct device *dev = kproc->dev;
> +
> +	mbox_free_channel(kproc->mbox);
> +	dev_info(dev, "R5F core deinitialized in IPC-only mode\n");
> +	return 0;
> +}
> +
> +/*
> + * This function implements the .get_loaded_rsc_table() callback and is used
> + * to provide the resource table for the booted R5F in IPC-only mode. The K3 R5F
> + * firmwares follow a design-by-contract approach and are expected to have the
> + * resource table at the base of the DDR region reserved for firmware usage.
> + * This provides flexibility for the remote processor to be booted by different
> + * bootloaders that may or may not have the ability to publish the resource table
> + * address and size through a DT property. This callback is invoked only in
> + * IPC-only mode.
> + */
> +static struct resource_table *k3_r5_get_loaded_rsc_table(struct rproc *rproc,
> +							 size_t *rsc_table_sz)
> +{
> +	struct k3_r5_rproc *kproc = rproc->priv;
> +	struct device *dev = kproc->dev;
> +
> +	if (!kproc->rmem[0].cpu_addr) {
> +		dev_err(dev, "memory-region #1 does not exist, loaded rsc table can't be found");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/*
> +	 * NOTE: The resource table size is currently hard-coded to a maximum
> +	 * of 256 bytes. The most common resource table usage for K3 firmwares
> +	 * is to only have the vdev resource entry and an optional trace entry.
> +	 * The exact size could be computed based on resource table address, but
> +	 * the hard-coded value suffices to support the IPC-only mode.
> +	 */
> +	*rsc_table_sz = 256;
> +	return (struct resource_table *)kproc->rmem[0].cpu_addr;
> +}
> +
>  /*
>   * Internal Memory translation helper
>   *
> @@ -1014,6 +1090,116 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
>  	}
>  }
>  
> +/*
> + * This function checks and configures a R5F core for IPC-only or remoteproc
> + * mode. The driver is configured to be in IPC-only mode for a R5F core when
> + * the core has been loaded and started by a bootloader. The IPC-only mode is
> + * detected by querying the System Firmware for reset, power on and halt status
> + * and ensuring that the core is running. Any incomplete steps at bootloader
> + * are validated and errored out.
> + *
> + * In IPC-only mode, the driver state flags for ATCM, BTCM and LOCZRAMA settings
> + * and cluster mode parsed originally from kernel DT are updated to reflect the
> + * actual values configured by bootloader. The driver internal device memory
> + * addresses for TCMs are also updated.
> + */
> +static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
> +{
> +	struct k3_r5_cluster *cluster = kproc->cluster;
> +	struct k3_r5_core *core = kproc->core;
> +	struct device *cdev = core->dev;
> +	bool r_state = false, c_state = false;
> +	u32 ctrl = 0, cfg = 0, stat = 0, halted = 0;
> +	u64 boot_vec = 0;
> +	u32 atcm_enable, btcm_enable, loczrama;
> +	struct k3_r5_core *core0;
> +	enum cluster_mode mode;
> +	int ret;
> +
> +	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
> +
> +	ret = core->ti_sci->ops.dev_ops.is_on(core->ti_sci, core->ti_sci_id,
> +					      &r_state, &c_state);
> +	if (ret) {
> +		dev_err(cdev, "failed to get initial state, mode cannot be determined, ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +	if (r_state != c_state) {
> +		dev_warn(cdev, "R5F core may have been powered on by a different host, programmed state (%d) != actual state (%d)\n",
> +			 r_state, c_state);
> +	}
> +
> +	ret = reset_control_status(core->reset);
> +	if (ret < 0) {
> +		dev_err(cdev, "failed to get initial local reset status, ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl,
> +				     &stat);
> +	if (ret < 0) {
> +		dev_err(cdev, "failed to get initial processor status, ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +	atcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_ATCM_EN ?  1 : 0;
> +	btcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_BTCM_EN ?  1 : 0;
> +	loczrama = cfg & PROC_BOOT_CFG_FLAG_R5_TCM_RSTBASE ?  1 : 0;
> +	if (cluster->soc_data->single_cpu_mode) {
> +		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
> +				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
> +	} else {
> +		mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
> +				CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;
> +	}
> +	halted = ctrl & PROC_BOOT_CTRL_FLAG_R5_CORE_HALT;
> +
> +	/*
> +	 * IPC-only mode detection requires both local and module resets to
> +	 * be deasserted and R5F core to be unhalted. Local reset status is
> +	 * irrelevant if module reset is asserted (POR value has local reset
> +	 * deasserted), and is deemed as remoteproc mode
> +	 */
> +	if (c_state && !ret && !halted) {
> +		dev_info(cdev, "configured R5F for IPC-only mode\n");
> +		kproc->rproc->state = RPROC_DETACHED;
> +		ret = 1;
> +		/* override rproc ops with only required IPC-only mode ops */
> +		kproc->rproc->ops->prepare = NULL;
> +		kproc->rproc->ops->unprepare = NULL;
> +		kproc->rproc->ops->start = NULL;
> +		kproc->rproc->ops->stop = NULL;
> +		kproc->rproc->ops->attach = k3_r5_rproc_attach;
> +		kproc->rproc->ops->detach = k3_r5_rproc_detach;
> +		kproc->rproc->ops->get_loaded_rsc_table =
> +						k3_r5_get_loaded_rsc_table;
> +	} else if (!c_state) {
> +		dev_info(cdev, "configured R5F for remoteproc mode\n");
> +		ret = 0;
> +	} else {
> +		dev_err(cdev, "mismatched mode: local_reset = %s, module_reset = %s, core_state = %s\n",
> +			!ret ? "deasserted" : "asserted",
> +			c_state ? "deasserted" : "asserted",
> +			halted ? "halted" : "unhalted");
> +		ret = -EINVAL;
> +	}
> +
> +	/* fixup TCMs, cluster & core flags to actual values in IPC-only mode */
> +	if (ret > 0) {
> +		if (core == core0)
> +			cluster->mode = mode;
> +		core->atcm_enable = atcm_enable;
> +		core->btcm_enable = btcm_enable;
> +		core->loczrama = loczrama;
> +		core->mem[0].dev_addr = loczrama ? 0 : K3_R5_TCM_DEV_ADDR;
> +		core->mem[1].dev_addr = loczrama ? K3_R5_TCM_DEV_ADDR : 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>  {
>  	struct k3_r5_cluster *cluster = platform_get_drvdata(pdev);
> @@ -1054,6 +1240,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>  		kproc->rproc = rproc;
>  		core->rproc = rproc;
>  
> +		ret = k3_r5_rproc_configure_mode(kproc);
> +		if (ret < 0)
> +			goto err_config;
> +		if (ret)
> +			goto init_rmem;
> +
>  		ret = k3_r5_rproc_configure(kproc);
>  		if (ret) {
>  			dev_err(dev, "initial configure failed, ret = %d\n",
> @@ -1061,6 +1253,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>  			goto err_config;
>  		}
>  
> +init_rmem:
>  		k3_r5_adjust_tcm_sizes(kproc);
>  
>  		ret = k3_r5_reserved_mem_init(kproc);
> -- 
> 2.32.0
> 

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

Re: [PATCH v2 5/5] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

[Mathieu Poirier]

On Fri, Jul 23, 2021 at 05:02:48PM -0500, Suman Anna wrote:
> Add support to the K3 DSP remoteproc driver to configure all the C66x
> and C71x cores on J721E SoCs to be either in IPC-only mode or the
> traditional remoteproc mode. The IPC-only mode expects that the remote
> processors are already booted by the bootloader, and only perform the
> minimum steps required to initialize and deinitialize the virtio IPC
> transports. The remoteproc mode allows the kernel remoteproc driver to
> do the regular load and boot and other device management operations for
> a DSP.
> 
> The IPC-only mode for a DSP is detected and configured at driver probe
> time by querying the System Firmware for the DSP power and reset state
> and/or status and making sure that the DSP is indeed started by the
> bootloaders, otherwise the device is configured for remoteproc mode.
> 
> Support for IPC-only mode is achieved through .attach(), .detach() and
> .get_loaded_rsc_table() callback ops and zeroing out the regular rproc
> ops .prepare(), .unprepare(), .start() and .stop(). The resource table
> follows a design-by-contract approach and is expected to be at the base
> of the DDR firmware region reserved for each remoteproc, it is mostly
> expected to contain only the virtio device and trace resource entries.
> 
> NOTE:
> The driver cannot configure a DSP core for remoteproc mode by any
> means without rebooting the kernel if that DSP core has been started
> by a bootloader.  This is the current desired behavior and can be
> enhanced in the future if the feature is needed.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: Addressed various review comments from v1
>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>    local ipc-only state flags
>  - Plugged in the required IPC-only ops dynamically with the regular
>    remoteproc-mode ops zeroed out
>  - Dropped all the unneeded error checks in start, stop, prepare, 
>    unprepare, attach and detach callbacks
>  - Callback function descriptions updated to reflect the mode they
>    apply to
>  - Dropped unused r_state variable in probe
>  - Switched to dev_info for the mode information traces from dev_err
>  - Revised the last 2 paras of the patch description
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-7-s-anna@ti.com/
> 
>  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 132 +++++++++++++++++++---
>  1 file changed, 115 insertions(+), 17 deletions(-)
> 

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> index faf60a274e8d..6eaecf02aee5 100644
> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -260,7 +260,8 @@ static int k3_dsp_rproc_request_mbox(struct rproc *rproc)
>   * used to release the global reset on C66x DSPs to allow loading into the DSP
>   * internal RAMs. The .prepare() ops is invoked by remoteproc core before any
>   * firmware loading, and is followed by the .start() ops after loading to
> - * actually let the C66x DSP cores run.
> + * actually let the C66x DSP cores run. This callback is invoked only in
> + * remoteproc mode.
>   */
>  static int k3_dsp_rproc_prepare(struct rproc *rproc)
>  {
> @@ -284,7 +285,7 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
>   * powering down the C66x DSP cores. The cores themselves are only halted in the
>   * .stop() callback through the local reset, and the .unprepare() ops is invoked
>   * by the remoteproc core after the remoteproc is stopped to balance the global
> - * reset.
> + * reset. This callback is invoked only in remoteproc mode.
>   */
>  static int k3_dsp_rproc_unprepare(struct rproc *rproc)
>  {
> @@ -305,7 +306,7 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
>   *
>   * This function will be invoked only after the firmware for this rproc
>   * was loaded, parsed successfully, and all of its resource requirements
> - * were met.
> + * were met. This callback is invoked only in remoteproc mode.
>   */
>  static int k3_dsp_rproc_start(struct rproc *rproc)
>  {
> @@ -346,7 +347,7 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>   * Stop the DSP remote processor.
>   *
>   * This function puts the DSP processor into reset, and finishes processing
> - * of any pending messages.
> + * of any pending messages. This callback is invoked only in remoteproc mode.
>   */
>  static int k3_dsp_rproc_stop(struct rproc *rproc)
>  {
> @@ -359,6 +360,78 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
>  	return 0;
>  }
>  
> +/*
> + * Attach to a running DSP remote processor (IPC-only mode)
> + *
> + * This rproc attach callback only needs to request the mailbox, the remote
> + * processor is already booted, so there is no need to issue any TI-SCI
> + * commands to boot the DSP core. This callback is invoked only in IPC-only
> + * mode.
> + */
> +static int k3_dsp_rproc_attach(struct rproc *rproc)
> +{
> +	struct k3_dsp_rproc *kproc = rproc->priv;
> +	struct device *dev = kproc->dev;
> +	int ret;
> +
> +	ret = k3_dsp_rproc_request_mbox(rproc);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "DSP initialized in IPC-only mode\n");
> +	return 0;
> +}
> +
> +/*
> + * Detach from a running DSP remote processor (IPC-only mode)
> + *
> + * This rproc detach callback performs the opposite operation to attach callback
> + * and only needs to release the mailbox, the DSP core is not stopped and will
> + * be left to continue to run its booted firmware. This callback is invoked only
> + * in IPC-only mode.
> + */
> +static int k3_dsp_rproc_detach(struct rproc *rproc)
> +{
> +	struct k3_dsp_rproc *kproc = rproc->priv;
> +	struct device *dev = kproc->dev;
> +
> +	mbox_free_channel(kproc->mbox);
> +	dev_info(dev, "DSP deinitialized in IPC-only mode\n");
> +	return 0;
> +}
> +
> +/*
> + * This function implements the .get_loaded_rsc_table() callback and is used
> + * to provide the resource table for a booted DSP in IPC-only mode. The K3 DSP
> + * firmwares follow a design-by-contract approach and are expected to have the
> + * resource table at the base of the DDR region reserved for firmware usage.
> + * This provides flexibility for the remote processor to be booted by different
> + * bootloaders that may or may not have the ability to publish the resource table
> + * address and size through a DT property. This callback is invoked only in
> + * IPC-only mode.
> + */
> +static struct resource_table *k3_dsp_get_loaded_rsc_table(struct rproc *rproc,
> +							  size_t *rsc_table_sz)
> +{
> +	struct k3_dsp_rproc *kproc = rproc->priv;
> +	struct device *dev = kproc->dev;
> +
> +	if (!kproc->rmem[0].cpu_addr) {
> +		dev_err(dev, "memory-region #1 does not exist, loaded rsc table can't be found");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/*
> +	 * NOTE: The resource table size is currently hard-coded to a maximum
> +	 * of 256 bytes. The most common resource table usage for K3 firmwares
> +	 * is to only have the vdev resource entry and an optional trace entry.
> +	 * The exact size could be computed based on resource table address, but
> +	 * the hard-coded value suffices to support the IPC-only mode.
> +	 */
> +	*rsc_table_sz = 256;
> +	return (struct resource_table *)kproc->rmem[0].cpu_addr;
> +}
> +
>  /*
>   * Custom function to translate a DSP device address (internal RAMs only) to a
>   * kernel virtual address.  The DSPs can access their RAMs at either an internal
> @@ -605,6 +678,7 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>  	struct k3_dsp_rproc *kproc;
>  	struct rproc *rproc;
>  	const char *fw_name;
> +	bool p_state = false;
>  	int ret = 0;
>  	int ret1;
>  
> @@ -683,19 +757,43 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>  		goto release_tsp;
>  	}
>  
> -	/*
> -	 * ensure the DSP local reset is asserted to ensure the DSP doesn't
> -	 * execute bogus code in .prepare() when the module reset is released.
> -	 */
> -	if (data->uses_lreset) {
> -		ret = reset_control_status(kproc->reset);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to get reset status, status = %d\n",
> -				ret);
> -			goto release_mem;
> -		} else if (ret == 0) {
> -			dev_warn(dev, "local reset is deasserted for device\n");
> -			k3_dsp_rproc_reset(kproc);
> +	ret = kproc->ti_sci->ops.dev_ops.is_on(kproc->ti_sci, kproc->ti_sci_id,
> +					       NULL, &p_state);
> +	if (ret) {
> +		dev_err(dev, "failed to get initial state, mode cannot be determined, ret = %d\n",
> +			ret);
> +		goto release_mem;
> +	}
> +
> +	/* configure J721E devices for either remoteproc or IPC-only mode */
> +	if (p_state) {
> +		dev_info(dev, "configured DSP for IPC-only mode\n");
> +		rproc->state = RPROC_DETACHED;
> +		/* override rproc ops with only required IPC-only mode ops */
> +		rproc->ops->prepare = NULL;
> +		rproc->ops->unprepare = NULL;
> +		rproc->ops->start = NULL;
> +		rproc->ops->stop = NULL;
> +		rproc->ops->attach = k3_dsp_rproc_attach;
> +		rproc->ops->detach = k3_dsp_rproc_detach;
> +		rproc->ops->get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table;
> +	} else {
> +		dev_info(dev, "configured DSP for remoteproc mode\n");
> +		/*
> +		 * ensure the DSP local reset is asserted to ensure the DSP
> +		 * doesn't execute bogus code in .prepare() when the module
> +		 * reset is released.
> +		 */
> +		if (data->uses_lreset) {
> +			ret = reset_control_status(kproc->reset);
> +			if (ret < 0) {
> +				dev_err(dev, "failed to get reset status, status = %d\n",
> +					ret);
> +				goto release_mem;
> +			} else if (ret == 0) {
> +				dev_warn(dev, "local reset is deasserted for device\n");
> +				k3_dsp_rproc_reset(kproc);
> +			}
>  		}
>  	}
>  
> -- 
> 2.32.0
> 

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

Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Mathieu Poirier]

On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
> The remoteproc core has support for both stopping and detaching a
> remote processor that was attached to previously, through both the
> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
> unconditionally only uses the stop functionality at present. This
> may not be the default desired functionality for all the remoteproc
> platform drivers.
> 
> Enhance the remoteproc core logic to key off the presence of the
> .stop() ops and allow the individual remoteproc drivers to continue
> to use the standard rproc_add() and rproc_del() API. This allows
> the remoteproc drivers to only do detach if supported when the driver
> is uninstalled, and the remote processor continues to run undisturbed
> even after the driver removal.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: Addressed various review comments from v1
>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>    rely only on rproc callback ops
>  - Updated the last para of the patch description
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
> 
>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index 4ad98b0b8caa..16c932beed88 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>  		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> +		if (rproc->state == RPROC_ATTACHED &&

This is already checked just above.

> +		    !rproc->ops->stop) {

This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
been provided. 

> +			dev_err(&rproc->dev,
> +				"stop not supported for this rproc, use detach\n");

The standard error message from the shell should be enough here, the same way it
is enough when the "start" and "stop" scenarios fail.

> +			return -EINVAL;
> +		}
> +
>  		rproc_shutdown(rproc);
>  	} else if (!strncmp(cmd, "detach", len)) {
>  		if (rproc->state != RPROC_ATTACHED)
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7de5905d276a..ab9e52180b04 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
>  	if (!atomic_dec_and_test(&rproc->power))
>  		goto out;
>  
> -	ret = rproc_stop(rproc, false);
> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
> +		ret = __rproc_detach(rproc);
> +	else
> +		ret = rproc_stop(rproc, false);

As I indicated in my last review I think rproc_shutdown() and rproc_del() should
be decoupled and the right call made in the platform drivers based on the state
of the remote processor.  Conditions such as the above make the core code
brittle, difficult to understand and tedious to maintain.

Thanks,
Mathieu

>  	if (ret) {
>  		atomic_inc(&rproc->power);
>  		goto out;
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index ea8b89f97d7b..133e766f38d4 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
>  		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> +		if (rproc->state == RPROC_ATTACHED &&
> +		    !rproc->ops->stop) {
> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
> +			return -EINVAL;
> +		}
> +
>  		rproc_shutdown(rproc);
>  	} else if (sysfs_streq(buf, "detach")) {
>  		if (rproc->state != RPROC_ATTACHED)
> -- 
> 2.32.0
> 

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

Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Suman Anna]

Hi Mathieu,

On 8/2/21 1:44 PM, Mathieu Poirier wrote:
> On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
>> The remoteproc core has support for both stopping and detaching a
>> remote processor that was attached to previously, through both the
>> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
>> unconditionally only uses the stop functionality at present. This
>> may not be the default desired functionality for all the remoteproc
>> platform drivers.
>>
>> Enhance the remoteproc core logic to key off the presence of the
>> .stop() ops and allow the individual remoteproc drivers to continue
>> to use the standard rproc_add() and rproc_del() API. This allows
>> the remoteproc drivers to only do detach if supported when the driver
>> is uninstalled, and the remote processor continues to run undisturbed
>> even after the driver removal.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> v2: Addressed various review comments from v1
>>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>>    rely only on rproc callback ops
>>  - Updated the last para of the patch description
>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
>>
>>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
>> index 4ad98b0b8caa..16c932beed88 100644
>> --- a/drivers/remoteproc/remoteproc_cdev.c
>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>>  		    rproc->state != RPROC_ATTACHED)
>>  			return -EINVAL;
>>  
>> +		if (rproc->state == RPROC_ATTACHED &&
> 
> This is already checked just above.
> 
>> +		    !rproc->ops->stop) {

Well, this is checking for both conditions, and not just the stop ops
independently. We expect to have .stop() defined normally for both regular
remoteproc mode and attached mode where you want to stop (and not detach), but
as you can see, I am supporting only detach and so will not have .stop() defined
 with RPROC_ATTACHED.

> 
> This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
> been provided.

rproc_shutdown() actually doesn't return any status, so all its internal
checking gets ignored and a success is returned today.

> 
>> +			dev_err(&rproc->dev,
>> +				"stop not supported for this rproc, use detach\n");
> 
> The standard error message from the shell should be enough here, the same way it
> is enough when the "start" and "stop" scenarios fail.

Thought this was a bit more informative, but sure this trace can be dropped.

> 
>> +			return -EINVAL;
>> +		}
>> +
>>  		rproc_shutdown(rproc);
>>  	} else if (!strncmp(cmd, "detach", len)) {
>>  		if (rproc->state != RPROC_ATTACHED)
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 7de5905d276a..ab9e52180b04 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
>>  	if (!atomic_dec_and_test(&rproc->power))
>>  		goto out;
>>  
>> -	ret = rproc_stop(rproc, false);
>> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
>> +		ret = __rproc_detach(rproc);
>> +	else
>> +		ret = rproc_stop(rproc, false);
> 
> As I indicated in my last review I think rproc_shutdown() and rproc_del() should
> be decoupled and the right call made in the platform drivers based on the state
> of the remote processor.  

We have various remoteproc API provided in pairs - rproc_alloc()/rproc_free(),
rproc_add()/rproc_del(), rproc_boot()/rproc_shutdown() and
rproc_attach()/rproc_detach(). The drivers are configuring conditions for
auto-boot and RPROC_DETACHED. The reason they are coupled is primarily because
of the auto-boot done during rproc_add(). And we handle the RPROC_DETACHED case
just as well in rproc_boot().

While what you have suggested works, but I am not quite convinced on this
asymmetric usage, and why this state-machine logic should be split between the
core and remoteproc drivers differently between attach and detach. To me,
calling rproc_detach() in remoteproc drivers would have made sense only if they
are also calling rproc_attach().


Conditions such as the above make the core code
> brittle, difficult to understand and tedious to maintain.

The logic I have added actually makes rproc_shutdown behavior to be on par with
the rproc_boot().

regards
Suman

> 
> Thanks,
> Mathieu
> 
>>  	if (ret) {
>>  		atomic_inc(&rproc->power);
>>  		goto out;
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> index ea8b89f97d7b..133e766f38d4 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
>>  		    rproc->state != RPROC_ATTACHED)
>>  			return -EINVAL;
>>  
>> +		if (rproc->state == RPROC_ATTACHED &&
>> +		    !rproc->ops->stop) {
>> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
>> +			return -EINVAL;
>> +		}
>> +
>>  		rproc_shutdown(rproc);
>>  	} else if (sysfs_streq(buf, "detach")) {
>>  		if (rproc->state != RPROC_ATTACHED)
>> -- 
>> 2.32.0
>>


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

Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Mathieu Poirier]

Good morning,

On Mon, Aug 02, 2021 at 06:21:38PM -0500, Suman Anna wrote:
> Hi Mathieu,
> 
> On 8/2/21 1:44 PM, Mathieu Poirier wrote:
> > On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
> >> The remoteproc core has support for both stopping and detaching a
> >> remote processor that was attached to previously, through both the
> >> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
> >> unconditionally only uses the stop functionality at present. This
> >> may not be the default desired functionality for all the remoteproc
> >> platform drivers.
> >>
> >> Enhance the remoteproc core logic to key off the presence of the
> >> .stop() ops and allow the individual remoteproc drivers to continue
> >> to use the standard rproc_add() and rproc_del() API. This allows
> >> the remoteproc drivers to only do detach if supported when the driver
> >> is uninstalled, and the remote processor continues to run undisturbed
> >> even after the driver removal.
> >>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> ---
> >> v2: Addressed various review comments from v1
> >>  - Reworked the logic to not use remoteproc detach_on_shutdown and
> >>    rely only on rproc callback ops
> >>  - Updated the last para of the patch description
> >> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
> >>
> >>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
> >>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
> >>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
> >>  3 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> >> index 4ad98b0b8caa..16c932beed88 100644
> >> --- a/drivers/remoteproc/remoteproc_cdev.c
> >> +++ b/drivers/remoteproc/remoteproc_cdev.c
> >> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
> >>  		    rproc->state != RPROC_ATTACHED)
> >>  			return -EINVAL;
> >>  
> >> +		if (rproc->state == RPROC_ATTACHED &&
> > 
> > This is already checked just above.
> > 
> >> +		    !rproc->ops->stop) {
> 
> Well, this is checking for both conditions, and not just the stop ops
> independently. We expect to have .stop() defined normally for both regular
> remoteproc mode and attached mode where you want to stop (and not detach), but
> as you can see, I am supporting only detach and so will not have .stop() defined
>  with RPROC_ATTACHED.
> 
> > 
> > This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
> > been provided.
> 
> rproc_shutdown() actually doesn't return any status, so all its internal
> checking gets ignored and a success is returned today.
> 

That is correct, and I have suggested to add a return value in my previous
review.

> > 
> >> +			dev_err(&rproc->dev,
> >> +				"stop not supported for this rproc, use detach\n");
> > 
> > The standard error message from the shell should be enough here, the same way it
> > is enough when the "start" and "stop" scenarios fail.
> 
> Thought this was a bit more informative, but sure this trace can be dropped.
> 
> > 
> >> +			return -EINVAL;
> >> +		}
> >> +
> >>  		rproc_shutdown(rproc);
> >>  	} else if (!strncmp(cmd, "detach", len)) {
> >>  		if (rproc->state != RPROC_ATTACHED)
> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >> index 7de5905d276a..ab9e52180b04 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
> >>  	if (!atomic_dec_and_test(&rproc->power))
> >>  		goto out;
> >>  
> >> -	ret = rproc_stop(rproc, false);
> >> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
> >> +		ret = __rproc_detach(rproc);
> >> +	else
> >> +		ret = rproc_stop(rproc, false);
> > 
> > As I indicated in my last review I think rproc_shutdown() and rproc_del() should
> > be decoupled and the right call made in the platform drivers based on the state
> > of the remote processor.  
> 
> We have various remoteproc API provided in pairs - rproc_alloc()/rproc_free(),
> rproc_add()/rproc_del(), rproc_boot()/rproc_shutdown() and
> rproc_attach()/rproc_detach(). The drivers are configuring conditions for
> auto-boot and RPROC_DETACHED. The reason they are coupled is primarily because
> of the auto-boot done during rproc_add(). And we handle the RPROC_DETACHED case
> just as well in rproc_boot().
>

The difference with rproc_boot() is that we are checking only the state of the
remoteproc, everything else related to the remote processor operations is
seamlessly handles by the state machine.  It is also tied to the
rproc_trigger_auto_boot() mechanic - decoupling that would be messy without
bringing any advantages other than keeping with a semantic symmetry.

> While what you have suggested works, but I am not quite convinced on this
> asymmetric usage, and why this state-machine logic should be split between the
> core and remoteproc drivers differently between attach and detach. To me,
> calling rproc_detach() in remoteproc drivers would have made sense only if they
> are also calling rproc_attach().

As pointed out above I see rproc_boot() as a special case but if that really
concerns you I'm open to consider patches that will take rproc_attach() out of
rproc_boot(). 

> 
> 
> Conditions such as the above make the core code
> > brittle, difficult to understand and tedious to maintain.
> 
> The logic I have added actually makes rproc_shutdown behavior to be on par with
> the rproc_boot().
> 
> regards
> Suman
> 
> > 
> > Thanks,
> > Mathieu
> > 
> >>  	if (ret) {
> >>  		atomic_inc(&rproc->power);
> >>  		goto out;
> >> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> >> index ea8b89f97d7b..133e766f38d4 100644
> >> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
> >>  		    rproc->state != RPROC_ATTACHED)
> >>  			return -EINVAL;
> >>  
> >> +		if (rproc->state == RPROC_ATTACHED &&
> >> +		    !rproc->ops->stop) {
> >> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
> >> +			return -EINVAL;
> >> +		}
> >> +
> >>  		rproc_shutdown(rproc);
> >>  	} else if (sysfs_streq(buf, "detach")) {
> >>  		if (rproc->state != RPROC_ATTACHED)
> >> -- 
> >> 2.32.0
> >>
> 

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

Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Suman Anna]

Hi Mathieu,

On 8/3/21 11:23 AM, Mathieu Poirier wrote:
> Good morning,
> 
> On Mon, Aug 02, 2021 at 06:21:38PM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>
>> On 8/2/21 1:44 PM, Mathieu Poirier wrote:
>>> On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
>>>> The remoteproc core has support for both stopping and detaching a
>>>> remote processor that was attached to previously, through both the
>>>> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
>>>> unconditionally only uses the stop functionality at present. This
>>>> may not be the default desired functionality for all the remoteproc
>>>> platform drivers.
>>>>
>>>> Enhance the remoteproc core logic to key off the presence of the
>>>> .stop() ops and allow the individual remoteproc drivers to continue
>>>> to use the standard rproc_add() and rproc_del() API. This allows
>>>> the remoteproc drivers to only do detach if supported when the driver
>>>> is uninstalled, and the remote processor continues to run undisturbed
>>>> even after the driver removal.
>>>>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> ---
>>>> v2: Addressed various review comments from v1
>>>>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>>>>    rely only on rproc callback ops
>>>>  - Updated the last para of the patch description
>>>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
>>>>
>>>>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>>>>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>>>>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>>>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
>>>> index 4ad98b0b8caa..16c932beed88 100644
>>>> --- a/drivers/remoteproc/remoteproc_cdev.c
>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>>> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>>>>  		    rproc->state != RPROC_ATTACHED)
>>>>  			return -EINVAL;
>>>>  
>>>> +		if (rproc->state == RPROC_ATTACHED &&
>>>
>>> This is already checked just above.
>>>
>>>> +		    !rproc->ops->stop) {
>>
>> Well, this is checking for both conditions, and not just the stop ops
>> independently. We expect to have .stop() defined normally for both regular
>> remoteproc mode and attached mode where you want to stop (and not detach), but
>> as you can see, I am supporting only detach and so will not have .stop() defined
>>  with RPROC_ATTACHED.
>>
>>>
>>> This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
>>> been provided.
>>
>> rproc_shutdown() actually doesn't return any status, so all its internal
>> checking gets ignored and a success is returned today.
>>
> 
> That is correct, and I have suggested to add a return value in my previous
> review.

Yeah ok. I can add a separate patch fixing that, and couple of these checks then
become redundant.

> 
>>>
>>>> +			dev_err(&rproc->dev,
>>>> +				"stop not supported for this rproc, use detach\n");
>>>
>>> The standard error message from the shell should be enough here, the same way it
>>> is enough when the "start" and "stop" scenarios fail.
>>
>> Thought this was a bit more informative, but sure this trace can be dropped.
>>
>>>
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>>  		rproc_shutdown(rproc);
>>>>  	} else if (!strncmp(cmd, "detach", len)) {
>>>>  		if (rproc->state != RPROC_ATTACHED)
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>> index 7de5905d276a..ab9e52180b04 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
>>>>  	if (!atomic_dec_and_test(&rproc->power))
>>>>  		goto out;
>>>>  
>>>> -	ret = rproc_stop(rproc, false);
>>>> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
>>>> +		ret = __rproc_detach(rproc);
>>>> +	else
>>>> +		ret = rproc_stop(rproc, false);
>>>
>>> As I indicated in my last review I think rproc_shutdown() and rproc_del() should
>>> be decoupled and the right call made in the platform drivers based on the state
>>> of the remote processor.  
>>
>> We have various remoteproc API provided in pairs - rproc_alloc()/rproc_free(),
>> rproc_add()/rproc_del(), rproc_boot()/rproc_shutdown() and
>> rproc_attach()/rproc_detach(). The drivers are configuring conditions for
>> auto-boot and RPROC_DETACHED. The reason they are coupled is primarily because
>> of the auto-boot done during rproc_add(). And we handle the RPROC_DETACHED case
>> just as well in rproc_boot().
>>
> 
> The difference with rproc_boot() is that we are checking only the state of the
> remoteproc, everything else related to the remote processor operations is
> seamlessly handles by the state machine.  It is also tied to the
> rproc_trigger_auto_boot() mechanic - decoupling that would be messy without
> bringing any advantages other than keeping with a semantic symmetry.

Most of this is actually tied to auto_boot if you think about it, not just the
rproc state. If we have auto_boot set to false, then rproc_add() would not do
anything, and the decision to start or attach can either be done through the
sysfs/cdev or a kernel remoteproc or some consumer driver. And the state machine
is getting influenced by this flag. auto-boot is a very useful feature.

You are asking is to do things differently between the regular start/stop case
and attach/detach case ignoring the auto-boot. The semantic symmetry actually
makes it easier to follow the state machine given that there are some internal
reference counts as well.

Note that we also have the devres API, and rproc_alloc()/rproc_free() and
rproc_add()/rproc_del() form the main remoteproc subsystem API. The drivers
would end up using matching calls if we don't have auto_boot.

> 
>> While what you have suggested works, but I am not quite convinced on this
>> asymmetric usage, and why this state-machine logic should be split between the
>> core and remoteproc drivers differently between attach and detach. To me,
>> calling rproc_detach() in remoteproc drivers would have made sense only if they
>> are also calling rproc_attach().
> 
> As pointed out above I see rproc_boot() as a special case but if that really
> concerns you I'm open to consider patches that will take rproc_attach() out of
> rproc_boot(). 
> 

We are talking about a bigger behavioral change to remoteproc core here. So I
would definitely want to hear from others as well on this before we spend any
time reworking code.

Meanwhile, how do I take this series forward? One option I can probably do is
turn off auto-boot for early-boot case in my drivers and do the matching
attach/detach.

regards
Suman

>>
>>
>> Conditions such as the above make the core code
>>> brittle, difficult to understand and tedious to maintain.
>>
>> The logic I have added actually makes rproc_shutdown behavior to be on par with
>> the rproc_boot().
>>
>> regards
>> Suman
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>>  	if (ret) {
>>>>  		atomic_inc(&rproc->power);
>>>>  		goto out;
>>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>>>> index ea8b89f97d7b..133e766f38d4 100644
>>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>>>> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
>>>>  		    rproc->state != RPROC_ATTACHED)
>>>>  			return -EINVAL;
>>>>  
>>>> +		if (rproc->state == RPROC_ATTACHED &&
>>>> +		    !rproc->ops->stop) {
>>>> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>>  		rproc_shutdown(rproc);
>>>>  	} else if (sysfs_streq(buf, "detach")) {
>>>>  		if (rproc->state != RPROC_ATTACHED)
>>>> -- 
>>>> 2.32.0
>>>>
>>


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

Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Mathieu Poirier]

On Wed, Aug 04, 2021 at 02:17:22PM -0500, Suman Anna wrote:
> Hi Mathieu,
> 
> On 8/3/21 11:23 AM, Mathieu Poirier wrote:
> > Good morning,
> > 
> > On Mon, Aug 02, 2021 at 06:21:38PM -0500, Suman Anna wrote:
> >> Hi Mathieu,
> >>
> >> On 8/2/21 1:44 PM, Mathieu Poirier wrote:
> >>> On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
> >>>> The remoteproc core has support for both stopping and detaching a
> >>>> remote processor that was attached to previously, through both the
> >>>> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
> >>>> unconditionally only uses the stop functionality at present. This
> >>>> may not be the default desired functionality for all the remoteproc
> >>>> platform drivers.
> >>>>
> >>>> Enhance the remoteproc core logic to key off the presence of the
> >>>> .stop() ops and allow the individual remoteproc drivers to continue
> >>>> to use the standard rproc_add() and rproc_del() API. This allows
> >>>> the remoteproc drivers to only do detach if supported when the driver
> >>>> is uninstalled, and the remote processor continues to run undisturbed
> >>>> even after the driver removal.
> >>>>
> >>>> Signed-off-by: Suman Anna <s-anna@ti.com>
> >>>> ---
> >>>> v2: Addressed various review comments from v1
> >>>>  - Reworked the logic to not use remoteproc detach_on_shutdown and
> >>>>    rely only on rproc callback ops
> >>>>  - Updated the last para of the patch description
> >>>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
> >>>>
> >>>>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
> >>>>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
> >>>>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
> >>>>  3 files changed, 17 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> >>>> index 4ad98b0b8caa..16c932beed88 100644
> >>>> --- a/drivers/remoteproc/remoteproc_cdev.c
> >>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
> >>>> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
> >>>>  		    rproc->state != RPROC_ATTACHED)
> >>>>  			return -EINVAL;
> >>>>  
> >>>> +		if (rproc->state == RPROC_ATTACHED &&
> >>>
> >>> This is already checked just above.
> >>>
> >>>> +		    !rproc->ops->stop) {
> >>
> >> Well, this is checking for both conditions, and not just the stop ops
> >> independently. We expect to have .stop() defined normally for both regular
> >> remoteproc mode and attached mode where you want to stop (and not detach), but
> >> as you can see, I am supporting only detach and so will not have .stop() defined
> >>  with RPROC_ATTACHED.
> >>
> >>>
> >>> This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
> >>> been provided.
> >>
> >> rproc_shutdown() actually doesn't return any status, so all its internal
> >> checking gets ignored and a success is returned today.
> >>
> > 
> > That is correct, and I have suggested to add a return value in my previous
> > review.
> 
> Yeah ok. I can add a separate patch fixing that, and couple of these checks then
> become redundant.
> 
> > 
> >>>
> >>>> +			dev_err(&rproc->dev,
> >>>> +				"stop not supported for this rproc, use detach\n");
> >>>
> >>> The standard error message from the shell should be enough here, the same way it
> >>> is enough when the "start" and "stop" scenarios fail.
> >>
> >> Thought this was a bit more informative, but sure this trace can be dropped.
> >>
> >>>
> >>>> +			return -EINVAL;
> >>>> +		}
> >>>> +
> >>>>  		rproc_shutdown(rproc);
> >>>>  	} else if (!strncmp(cmd, "detach", len)) {
> >>>>  		if (rproc->state != RPROC_ATTACHED)
> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>>> index 7de5905d276a..ab9e52180b04 100644
> >>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
> >>>>  	if (!atomic_dec_and_test(&rproc->power))
> >>>>  		goto out;
> >>>>  
> >>>> -	ret = rproc_stop(rproc, false);
> >>>> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
> >>>> +		ret = __rproc_detach(rproc);
> >>>> +	else
> >>>> +		ret = rproc_stop(rproc, false);
> >>>
> >>> As I indicated in my last review I think rproc_shutdown() and rproc_del() should
> >>> be decoupled and the right call made in the platform drivers based on the state
> >>> of the remote processor.  
> >>
> >> We have various remoteproc API provided in pairs - rproc_alloc()/rproc_free(),
> >> rproc_add()/rproc_del(), rproc_boot()/rproc_shutdown() and
> >> rproc_attach()/rproc_detach(). The drivers are configuring conditions for
> >> auto-boot and RPROC_DETACHED. The reason they are coupled is primarily because
> >> of the auto-boot done during rproc_add(). And we handle the RPROC_DETACHED case
> >> just as well in rproc_boot().
> >>
> > 
> > The difference with rproc_boot() is that we are checking only the state of the
> > remoteproc, everything else related to the remote processor operations is
> > seamlessly handles by the state machine.  It is also tied to the
> > rproc_trigger_auto_boot() mechanic - decoupling that would be messy without
> > bringing any advantages other than keeping with a semantic symmetry.
> 
> Most of this is actually tied to auto_boot if you think about it, not just the
> rproc state. If we have auto_boot set to false, then rproc_add() would not do
> anything, and the decision to start or attach can either be done through the
> sysfs/cdev or a kernel remoteproc or some consumer driver. And the state machine
> is getting influenced by this flag. auto-boot is a very useful feature.
> 
> You are asking is to do things differently between the regular start/stop case
> and attach/detach case ignoring the auto-boot. The semantic symmetry actually
> makes it easier to follow the state machine given that there are some internal
> reference counts as well.

I am definitely not asking to ignore the auto-boot flag.  All I said is that I
did not split the semantic in rproc_boot() because of the auto-boot flag and the
mechanic to handle it.

> 
> Note that we also have the devres API, and rproc_alloc()/rproc_free() and
> rproc_add()/rproc_del() form the main remoteproc subsystem API. The drivers
> would end up using matching calls if we don't have auto_boot.
> 
> > 
> >> While what you have suggested works, but I am not quite convinced on this
> >> asymmetric usage, and why this state-machine logic should be split between the
> >> core and remoteproc drivers differently between attach and detach. To me,
> >> calling rproc_detach() in remoteproc drivers would have made sense only if they
> >> are also calling rproc_attach().
> > 
> > As pointed out above I see rproc_boot() as a special case but if that really
> > concerns you I'm open to consider patches that will take rproc_attach() out of
> > rproc_boot(). 
> > 
> 
> We are talking about a bigger behavioral change to remoteproc core here. So I
> would definitely want to hear from others as well on this before we spend any
> time reworking code.
> 
> Meanwhile, how do I take this series forward? One option I can probably do is
> turn off auto-boot for early-boot case in my drivers and do the matching
> attach/detach.
>

I don't think there is a need to turn off auto-boot for early boot, rproc_boot()
will to the right thing.

As for the way forward, the easiest way I see is to call either rproc_shutdown()
or rproc_detach() based on rproc->state in rproc_del().  That will work with
devm_rproc_remove() and it is still possible for platorm drivers to explicitly
call rproc_shutdown() before rproc_del() to force a remote processor that was
attached to be switched off when the driver is removed.

That is all the time I had for remoteproc - I am officially away for the next two weeks.  

Thanks,
Mathieu

> regards
> Suman
> 
> >>
> >>
> >> Conditions such as the above make the core code
> >>> brittle, difficult to understand and tedious to maintain.
> >>
> >> The logic I have added actually makes rproc_shutdown behavior to be on par with
> >> the rproc_boot().
> >>
> >> regards
> >> Suman
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>>  	if (ret) {
> >>>>  		atomic_inc(&rproc->power);
> >>>>  		goto out;
> >>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> >>>> index ea8b89f97d7b..133e766f38d4 100644
> >>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >>>> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
> >>>>  		    rproc->state != RPROC_ATTACHED)
> >>>>  			return -EINVAL;
> >>>>  
> >>>> +		if (rproc->state == RPROC_ATTACHED &&
> >>>> +		    !rproc->ops->stop) {
> >>>> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
> >>>> +			return -EINVAL;
> >>>> +		}
> >>>> +
> >>>>  		rproc_shutdown(rproc);
> >>>>  	} else if (sysfs_streq(buf, "detach")) {
> >>>>  		if (rproc->state != RPROC_ATTACHED)
> >>>> -- 
> >>>> 2.32.0
> >>>>
> >>
> 

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

Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Suman Anna]

On 8/5/21 12:35 PM, Mathieu Poirier wrote:
> On Wed, Aug 04, 2021 at 02:17:22PM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>
>> On 8/3/21 11:23 AM, Mathieu Poirier wrote:
>>> Good morning,
>>>
>>> On Mon, Aug 02, 2021 at 06:21:38PM -0500, Suman Anna wrote:
>>>> Hi Mathieu,
>>>>
>>>> On 8/2/21 1:44 PM, Mathieu Poirier wrote:
>>>>> On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
>>>>>> The remoteproc core has support for both stopping and detaching a
>>>>>> remote processor that was attached to previously, through both the
>>>>>> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
>>>>>> unconditionally only uses the stop functionality at present. This
>>>>>> may not be the default desired functionality for all the remoteproc
>>>>>> platform drivers.
>>>>>>
>>>>>> Enhance the remoteproc core logic to key off the presence of the
>>>>>> .stop() ops and allow the individual remoteproc drivers to continue
>>>>>> to use the standard rproc_add() and rproc_del() API. This allows
>>>>>> the remoteproc drivers to only do detach if supported when the driver
>>>>>> is uninstalled, and the remote processor continues to run undisturbed
>>>>>> even after the driver removal.
>>>>>>
>>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>>>> ---
>>>>>> v2: Addressed various review comments from v1
>>>>>>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>>>>>>    rely only on rproc callback ops
>>>>>>  - Updated the last para of the patch description
>>>>>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
>>>>>>
>>>>>>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>>>>>>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>>>>>>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>>>>>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
>>>>>> index 4ad98b0b8caa..16c932beed88 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_cdev.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>>>>> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>>>>>>  		    rproc->state != RPROC_ATTACHED)
>>>>>>  			return -EINVAL;
>>>>>>  
>>>>>> +		if (rproc->state == RPROC_ATTACHED &&
>>>>>
>>>>> This is already checked just above.
>>>>>
>>>>>> +		    !rproc->ops->stop) {
>>>>
>>>> Well, this is checking for both conditions, and not just the stop ops
>>>> independently. We expect to have .stop() defined normally for both regular
>>>> remoteproc mode and attached mode where you want to stop (and not detach), but
>>>> as you can see, I am supporting only detach and so will not have .stop() defined
>>>>  with RPROC_ATTACHED.
>>>>
>>>>>
>>>>> This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
>>>>> been provided.
>>>>
>>>> rproc_shutdown() actually doesn't return any status, so all its internal
>>>> checking gets ignored and a success is returned today.
>>>>
>>>
>>> That is correct, and I have suggested to add a return value in my previous
>>> review.
>>
>> Yeah ok. I can add a separate patch fixing that, and couple of these checks then
>> become redundant.
>>
>>>
>>>>>
>>>>>> +			dev_err(&rproc->dev,
>>>>>> +				"stop not supported for this rproc, use detach\n");
>>>>>
>>>>> The standard error message from the shell should be enough here, the same way it
>>>>> is enough when the "start" and "stop" scenarios fail.
>>>>
>>>> Thought this was a bit more informative, but sure this trace can be dropped.
>>>>
>>>>>
>>>>>> +			return -EINVAL;
>>>>>> +		}
>>>>>> +
>>>>>>  		rproc_shutdown(rproc);
>>>>>>  	} else if (!strncmp(cmd, "detach", len)) {
>>>>>>  		if (rproc->state != RPROC_ATTACHED)
>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>>>> index 7de5905d276a..ab9e52180b04 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
>>>>>>  	if (!atomic_dec_and_test(&rproc->power))
>>>>>>  		goto out;
>>>>>>  
>>>>>> -	ret = rproc_stop(rproc, false);
>>>>>> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
>>>>>> +		ret = __rproc_detach(rproc);
>>>>>> +	else
>>>>>> +		ret = rproc_stop(rproc, false);
>>>>>
>>>>> As I indicated in my last review I think rproc_shutdown() and rproc_del() should
>>>>> be decoupled and the right call made in the platform drivers based on the state
>>>>> of the remote processor.  
>>>>
>>>> We have various remoteproc API provided in pairs - rproc_alloc()/rproc_free(),
>>>> rproc_add()/rproc_del(), rproc_boot()/rproc_shutdown() and
>>>> rproc_attach()/rproc_detach(). The drivers are configuring conditions for
>>>> auto-boot and RPROC_DETACHED. The reason they are coupled is primarily because
>>>> of the auto-boot done during rproc_add(). And we handle the RPROC_DETACHED case
>>>> just as well in rproc_boot().
>>>>
>>>
>>> The difference with rproc_boot() is that we are checking only the state of the
>>> remoteproc, everything else related to the remote processor operations is
>>> seamlessly handles by the state machine.  It is also tied to the
>>> rproc_trigger_auto_boot() mechanic - decoupling that would be messy without
>>> bringing any advantages other than keeping with a semantic symmetry.
>>
>> Most of this is actually tied to auto_boot if you think about it, not just the
>> rproc state. If we have auto_boot set to false, then rproc_add() would not do
>> anything, and the decision to start or attach can either be done through the
>> sysfs/cdev or a kernel remoteproc or some consumer driver. And the state machine
>> is getting influenced by this flag. auto-boot is a very useful feature.
>>
>> You are asking is to do things differently between the regular start/stop case
>> and attach/detach case ignoring the auto-boot. The semantic symmetry actually
>> makes it easier to follow the state machine given that there are some internal
>> reference counts as well.
> 
> I am definitely not asking to ignore the auto-boot flag.  All I said is that I
> did not split the semantic in rproc_boot() because of the auto-boot flag and the
> mechanic to handle it.
> 
>>
>> Note that we also have the devres API, and rproc_alloc()/rproc_free() and
>> rproc_add()/rproc_del() form the main remoteproc subsystem API. The drivers
>> would end up using matching calls if we don't have auto_boot.
>>
>>>
>>>> While what you have suggested works, but I am not quite convinced on this
>>>> asymmetric usage, and why this state-machine logic should be split between the
>>>> core and remoteproc drivers differently between attach and detach. To me,
>>>> calling rproc_detach() in remoteproc drivers would have made sense only if they
>>>> are also calling rproc_attach().
>>>
>>> As pointed out above I see rproc_boot() as a special case but if that really
>>> concerns you I'm open to consider patches that will take rproc_attach() out of
>>> rproc_boot(). 
>>>
>>
>> We are talking about a bigger behavioral change to remoteproc core here. So I
>> would definitely want to hear from others as well on this before we spend any
>> time reworking code.
>>
>> Meanwhile, how do I take this series forward? One option I can probably do is
>> turn off auto-boot for early-boot case in my drivers and do the matching
>> attach/detach.
>>
> 
> I don't think there is a need to turn off auto-boot for early boot, rproc_boot()
> will to the right thing.
> 
> As for the way forward, the easiest way I see is to call either rproc_shutdown()
> or rproc_detach() based on rproc->state in rproc_del().  That will work with
> devm_rproc_remove() and it is still possible for platorm drivers to explicitly
> call rproc_shutdown() before rproc_del() to force a remote processor that was
> attached to be switched off when the driver is removed.
> 
> That is all the time I had for remoteproc - I am officially away for the next two weeks.  

Yeah, I can make these modification. Let me spin a v3 with this, most probably
waiting for you when you come back :)

regards
Suman

> 
> Thanks,
> Mathieu
> 
>> regards
>> Suman
>>
>>>>
>>>>
>>>> Conditions such as the above make the core code
>>>>> brittle, difficult to understand and tedious to maintain.
>>>>
>>>> The logic I have added actually makes rproc_shutdown behavior to be on par with
>>>> the rproc_boot().
>>>>
>>>> regards
>>>> Suman
>>>>
>>>>>
>>>>> Thanks,
>>>>> Mathieu
>>>>>
>>>>>>  	if (ret) {
>>>>>>  		atomic_inc(&rproc->power);
>>>>>>  		goto out;
>>>>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>>>>>> index ea8b89f97d7b..133e766f38d4 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>>>>>> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
>>>>>>  		    rproc->state != RPROC_ATTACHED)
>>>>>>  			return -EINVAL;
>>>>>>  
>>>>>> +		if (rproc->state == RPROC_ATTACHED &&
>>>>>> +		    !rproc->ops->stop) {
>>>>>> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
>>>>>> +			return -EINVAL;
>>>>>> +		}
>>>>>> +
>>>>>>  		rproc_shutdown(rproc);
>>>>>>  	} else if (sysfs_streq(buf, "detach")) {
>>>>>>  		if (rproc->state != RPROC_ATTACHED)
>>>>>> -- 
>>>>>> 2.32.0
>>>>>>
>>>>
>>


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

Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown

[Arnaud POULIQUEN]

Hi Suman, Mathieu,


On 8/4/21 9:17 PM, Suman Anna wrote:
> Hi Mathieu,
> 
> On 8/3/21 11:23 AM, Mathieu Poirier wrote:
>> Good morning,
>>
>> On Mon, Aug 02, 2021 at 06:21:38PM -0500, Suman Anna wrote:
>>> Hi Mathieu,
>>>
>>> On 8/2/21 1:44 PM, Mathieu Poirier wrote:
>>>> On Fri, Jul 23, 2021 at 05:02:44PM -0500, Suman Anna wrote:
>>>>> The remoteproc core has support for both stopping and detaching a
>>>>> remote processor that was attached to previously, through both the
>>>>> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
>>>>> unconditionally only uses the stop functionality at present. This
>>>>> may not be the default desired functionality for all the remoteproc
>>>>> platform drivers.
>>>>>
>>>>> Enhance the remoteproc core logic to key off the presence of the
>>>>> .stop() ops and allow the individual remoteproc drivers to continue
>>>>> to use the standard rproc_add() and rproc_del() API. This allows
>>>>> the remoteproc drivers to only do detach if supported when the driver
>>>>> is uninstalled, and the remote processor continues to run undisturbed
>>>>> even after the driver removal.
>>>>>
>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>>> ---
>>>>> v2: Addressed various review comments from v1
>>>>>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>>>>>    rely only on rproc callback ops
>>>>>  - Updated the last para of the patch description
>>>>> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
>>>>>
>>>>>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>>>>>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>>>>>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>>>>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
>>>>> index 4ad98b0b8caa..16c932beed88 100644
>>>>> --- a/drivers/remoteproc/remoteproc_cdev.c
>>>>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>>>>> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>>>>>  		    rproc->state != RPROC_ATTACHED)
>>>>>  			return -EINVAL;
>>>>>  
>>>>> +		if (rproc->state == RPROC_ATTACHED &&
>>>>
>>>> This is already checked just above.
>>>>
>>>>> +		    !rproc->ops->stop) {
>>>
>>> Well, this is checking for both conditions, and not just the stop ops
>>> independently. We expect to have .stop() defined normally for both regular
>>> remoteproc mode and attached mode where you want to stop (and not detach), but
>>> as you can see, I am supporting only detach and so will not have .stop() defined
>>>  with RPROC_ATTACHED.
>>>
>>>>
>>>> This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
>>>> been provided.
>>>
>>> rproc_shutdown() actually doesn't return any status, so all its internal
>>> checking gets ignored and a success is returned today.
>>>
>>
>> That is correct, and I have suggested to add a return value in my previous
>> review.
> 
> Yeah ok. I can add a separate patch fixing that, and couple of these checks then
> become redundant.
> 
>>
>>>>
>>>>> +			dev_err(&rproc->dev,
>>>>> +				"stop not supported for this rproc, use detach\n");
>>>>
>>>> The standard error message from the shell should be enough here, the same way it
>>>> is enough when the "start" and "stop" scenarios fail.
>>>
>>> Thought this was a bit more informative, but sure this trace can be dropped.
>>>
>>>>
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +
>>>>>  		rproc_shutdown(rproc);
>>>>>  	} else if (!strncmp(cmd, "detach", len)) {
>>>>>  		if (rproc->state != RPROC_ATTACHED)
>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>>> index 7de5905d276a..ab9e52180b04 100644
>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
>>>>>  	if (!atomic_dec_and_test(&rproc->power))
>>>>>  		goto out;
>>>>>  
>>>>> -	ret = rproc_stop(rproc, false);
>>>>> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
>>>>> +		ret = __rproc_detach(rproc);
>>>>> +	else
>>>>> +		ret = rproc_stop(rproc, false);
>>>>
>>>> As I indicated in my last review I think rproc_shutdown() and rproc_del() should
>>>> be decoupled and the right call made in the platform drivers based on the state
>>>> of the remote processor.  

Agree With Mathieu. More than that it looks to me that the stop ops is not
optional and mandatory to implement the remoteproc shutdown. It should be the
role of the platform driver to decide if a stop is a detachment.

This behavior may also depend on the project for multi purpose platforms. In
this case DT property might be more appropriate than a null stop ops to
determine the behavior.

>>>
>>> We have various remoteproc API provided in pairs - rproc_alloc()/rproc_free(),
>>> rproc_add()/rproc_del(), rproc_boot()/rproc_shutdown() and
>>> rproc_attach()/rproc_detach(). The drivers are configuring conditions for
>>> auto-boot and RPROC_DETACHED. The reason they are coupled is primarily because
>>> of the auto-boot done during rproc_add(). And we handle the RPROC_DETACHED case
>>> just as well in rproc_boot().
>>>
>>
>> The difference with rproc_boot() is that we are checking only the state of the
>> remoteproc, everything else related to the remote processor operations is
>> seamlessly handles by the state machine.  It is also tied to the
>> rproc_trigger_auto_boot() mechanic - decoupling that would be messy without
>> bringing any advantages other than keeping with a semantic symmetry.
> 
> Most of this is actually tied to auto_boot if you think about it, not just the
> rproc state. If we have auto_boot set to false, then rproc_add() would not do
> anything, and the decision to start or attach can either be done through the
> sysfs/cdev or a kernel remoteproc or some consumer driver. And the state machine
> is getting influenced by this flag. auto-boot is a very useful feature.
> 
> You are asking is to do things differently between the regular start/stop case
> and attach/detach case ignoring the auto-boot. The semantic symmetry actually
> makes it easier to follow the state machine given that there are some internal
> reference counts as well.
> 
> Note that we also have the devres API, and rproc_alloc()/rproc_free() and
> rproc_add()/rproc_del() form the main remoteproc subsystem API. The drivers
> would end up using matching calls if we don't have auto_boot.
> 
>>
>>> While what you have suggested works, but I am not quite convinced on this
>>> asymmetric usage, and why this state-machine logic should be split between the
>>> core and remoteproc drivers differently between attach and detach. To me,
>>> calling rproc_detach() in remoteproc drivers would have made sense only if they
>>> are also calling rproc_attach().

In current implementation to be able to detach a remote processor we need to be
in RPROC_ATTACHED state. If I well remember the reason is the backup of the
resource table to reinitialize it on detach.should we improve this?

We could also consider the attach and detach as 2 separate features:
- attach is used for a pre loaded firmware (e.g early action use case waiting
Linux boot)
   => can be stopped as a loaded firmware.

- boot a remote firmware loaded from sysfs but detach it on Linux shutdown to
allow it to property stop it own activities (e.g Linux independent reboot).

>>
>> As pointed out above I see rproc_boot() as a special case but if that really
>> concerns you I'm open to consider patches that will take rproc_attach() out of
>> rproc_boot(). 
>>
> 
> We are talking about a bigger behavioral change to remoteproc core here. So I
> would definitely want to hear from others as well on this before we spend any
> time reworking code.

I'm not sure to understand what would be behind this rework. What is exactly the
issue on the rproc_boot? having a semantic symmetry? In this case do you expect
that the user application determines the current state of the remote processor
and "start" it or "attach" it, depending on state? Or does it be implemented in
rproc_sysfs and rpcroc_cdev (and few platform drivers).

Look to me that take rproc_attach() out of rproc_boot() could break or
complexify some legacy APIs.

Regards,
Arnaud

> 
> Meanwhile, how do I take this series forward? One option I can probably do is
> turn off auto-boot for early-boot case in my drivers and do the matching
> attach/detach.
> 
> regards
> Suman
> 
>>>
>>>
>>> Conditions such as the above make the core code
>>>> brittle, difficult to understand and tedious to maintain.
>>>
>>> The logic I have added actually makes rproc_shutdown behavior to be on par with
>>> the rproc_boot().
>>>
>>> regards
>>> Suman
>>>
>>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>>>  	if (ret) {
>>>>>  		atomic_inc(&rproc->power);
>>>>>  		goto out;
>>>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>>>>> index ea8b89f97d7b..133e766f38d4 100644
>>>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>>>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>>>>> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
>>>>>  		    rproc->state != RPROC_ATTACHED)
>>>>>  			return -EINVAL;
>>>>>  
>>>>> +		if (rproc->state == RPROC_ATTACHED &&
>>>>> +		    !rproc->ops->stop) {
>>>>> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +
>>>>>  		rproc_shutdown(rproc);
>>>>>  	} else if (sysfs_streq(buf, "detach")) {
>>>>>  		if (rproc->state != RPROC_ATTACHED)
>>>>> -- 
>>>>> 2.32.0
>>>>>
>>>
> 

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