2021-05-22 00:05:17

by Suman Anna

[permalink] [raw]
Subject: [PATCH 0/6] K3 R5F & DSP IPC-only mode support

Hi All,

The following series 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.13-rc1 + Minor remoteproc cleanup
series [1] + TI K3 R5F remoteproc support on AM64x series [2].

The 'IPC-only' mode terminology essentially means establishing only the
IPC rpmsg stack for remoteprocs that are booted early by bootloaders and
supporting only 'attach' and 'detach' of the remoteprocs. The existing
remoteproc infrastructure does support 'stop' of an early-booted remote
processor, and the TI K3 remoteprocs are intentionally being limited to
'detach', and are designed to only establish and tear apart IPC (the
virtio devices that provide the IPC transport) without ever shutting
down the core. This is done by introducing a new flag in remoteproc core
in Patch 2. Support for the regular 'stop' can be enhanced in the future
easily by adding a new sysfs or configfs file that changes this flag,
but IPC-only is the expected usage model for now on K3 SoCs.

Following is a summary of some of the design details:
- The TI K3 SoCs use a dedicated system processor for Power and Clocks
and the IPC-only mode is detected by communicating with this central
processor and checking on the power status of remoteprocs.
- The driver support is provided through the recently added .attach(),
.detach() and .get_loaded_rsc_table() rproc ops, and new RPROC_ATTACHED
and RPROC_DETACHED state flags.
- The default kernel dts cluster modes and TCM configuration for R5Fs
are ignored and overridden by querying this config from the system
processor in IPC-only mode. Any core not booted earlier will fall back
to using the kernel dts (eg: R5F Core0 can be booted by bootloader, and
R5F Core1 will be booted using kernel).
- The remoteproc firmwares are not loaded again in kernel to retrieve
the resource table. This allows early-boot to be done from different
boot media such as OSPI or eMMC, and be completely independent of the
rootfs in SDCard or NFS.
- The IPC-only mode support follows a design-by-contract (DBC) approach
to achieve the above. The resource tables are always expected to be at
the base on the DDR memory region reserved for firmwares (base of the
second memory-region property in the remoteproc dts node). This also
eliminates the need for dts properties being dynamically updated to
provide the resource table address (there are no dedicated h/w
registers either like with the ST SoCs). This scales to designs where
either U-Boot is not involved or a different processor is used to
load the various remoteprocs.
- The "stop" command results in a failure when the remoteproc is in
IPC-only mode, and "detach" serves as the corresponding replacement.
Note that there is no equivalent "attach" command, and "start" command
also serves as "attach" for remoteprocs that are in detached state.
- The remoteproc sysfs 'state' file shows the mode as "detached"
when the early-booted remoteproc is detached. "offline" continues
to be shown when the remoteproc is powered off.

Following is the summary of patches:
- Patch 1 is a minor cleanup patch in remoteproc core to make the
helper functions look symmetric.
- Patch 2 introduces a new flag 'detach_on_shutdown' and enhances
the remoteproc core to restrict stop on early-booted remoteprocs.
This patch provides the required behavior for K3 IPC-only mode (no
stopping of early-booted rprocs). The default behavior continues
to support stopping of remoteproc like with the existing code.
- Patches 3 and 5 refactor the mailbox request code out of start
for reuse in the new attach callbacks.
- Patch 4 adds the IPC-only mode support for R5F.
- Patch 6 adds the IPC-only mode support for both K3 C66x and C71x
DSPs.

Patches 1, 3 and 5 can be considered cleanup, so I suggest these be
picked up if there are any major comments on the others.

I have 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/list/?series=485235
[2] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=456755

Suman Anna (6):
remoteproc: Introduce rproc_detach_device() wrapper
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 | 7 +-
drivers/remoteproc/remoteproc_internal.h | 8 +
drivers/remoteproc/remoteproc_sysfs.c | 6 +
drivers/remoteproc/ti_k3_dsp_remoteproc.c | 214 ++++++++++++++---
drivers/remoteproc/ti_k3_r5_remoteproc.c | 279 ++++++++++++++++++++--
include/linux/remoteproc.h | 3 +
7 files changed, 459 insertions(+), 65 deletions(-)

--
2.30.1


2021-05-22 00:06:12

by Suman Anna

[permalink] [raw]
Subject: [PATCH 2/6] remoteproc: Add support for detach-only during shutdown

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.

Introduce a new rproc state flag 'detach_on_shutdown' that individual
remoteproc drivers can set to only allow detach in rproc_shutdown()
that would have been invoked when the driver is uninstalled, so that
remote processor continues to run undisturbed even after the driver
removal.

Signed-off-by: Suman Anna <[email protected]>
---
drivers/remoteproc/remoteproc_cdev.c | 7 +++++++
drivers/remoteproc/remoteproc_core.c | 5 ++++-
drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
include/linux/remoteproc.h | 3 +++
4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 0b8a84c04f76..473467711a09 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->detach_on_shutdown) {
+ 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 6019f46001c8..e8ab3eb41f00 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
if (!atomic_dec_and_test(&rproc->power))
goto out;

- ret = rproc_stop(rproc, false);
+ if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
+ 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..1785fbcb1075 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->detach_on_shutdown) {
+ 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)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 42a1f30e33a7..35ef921676a1 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -530,6 +530,8 @@ struct rproc_dump_segment {
* @elf_machine: firmware ELF machine
* @cdev: character device of the rproc
* @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
+ * attached state and _only_ support detach
*/
struct rproc {
struct list_head node;
@@ -569,6 +571,7 @@ struct rproc {
u16 elf_machine;
struct cdev cdev;
bool cdev_put_on_release;
+ bool detach_on_shutdown;
};

/**
--
2.30.1

2021-05-22 00:06:18

by Suman Anna

[permalink] [raw]
Subject: [PATCH 5/6] remoteproc: k3-dsp: Refactor mbox request code in start

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 <[email protected]>
---
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.30.1

2021-05-22 00:06:46

by Suman Anna

[permalink] [raw]
Subject: [PATCH 4/6] remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs

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 various other flags in both
remoteproc core and the K3 R5F remoteproc driver. 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.

Signed-off-by: Suman Anna <[email protected]>
---
drivers/remoteproc/ti_k3_r5_remoteproc.c | 215 +++++++++++++++++++++++
1 file changed, 215 insertions(+)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index e7e1ca71763e..66f0654f0860 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -151,6 +151,7 @@ struct k3_r5_core {
* @core: cached pointer to r5 core structure being used
* @rmem: reserved memory regions data
* @num_rmems: number of reserved memory regions
+ * @ipc_only: flag to indicate IPC-only mode
*/
struct k3_r5_rproc {
struct device *dev;
@@ -161,6 +162,7 @@ struct k3_r5_rproc {
struct k3_r5_core *core;
struct k3_r5_mem *rmem;
int num_rmems;
+ bool ipc_only;
};

/**
@@ -440,6 +442,10 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
bool mem_init_dis;
int ret;

+ /* IPC-only mode does not require the cores to be released from reset */
+ if (kproc->ipc_only)
+ return 0;
+
ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat);
if (ret < 0)
return ret;
@@ -503,6 +509,10 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc)
struct device *dev = kproc->dev;
int ret;

+ /* do not put back the cores into reset in IPC-only mode */
+ if (kproc->ipc_only)
+ return 0;
+
/* Re-use LockStep-mode reset logic for Single-CPU mode */
ret = (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
cluster->mode == CLUSTER_MODE_SINGLECPU) ?
@@ -538,6 +548,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
u32 boot_addr;
int ret;

+ if (kproc->ipc_only) {
+ dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
+ __func__);
+ return -EINVAL;
+ }
+
ret = k3_r5_rproc_request_mbox(rproc);
if (ret)
return ret;
@@ -604,9 +620,16 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
{
struct k3_r5_rproc *kproc = rproc->priv;
struct k3_r5_cluster *cluster = kproc->cluster;
+ struct device *dev = kproc->dev;
struct k3_r5_core *core = kproc->core;
int ret;

+ if (kproc->ipc_only) {
+ dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
+ __func__);
+ return -EINVAL;
+ }
+
/* halt all applicable cores */
if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
list_for_each_entry(core, &cluster->cores, elem) {
@@ -635,6 +658,85 @@ 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.
+ */
+static int k3_r5_rproc_attach(struct rproc *rproc)
+{
+ struct k3_r5_rproc *kproc = rproc->priv;
+ struct device *dev = kproc->dev;
+ int ret;
+
+ if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
+ dev_err(dev, "R5F is expected to be in IPC-only mode and RPROC_DETACHED state\n");
+ return -EINVAL;
+ }
+
+ ret = k3_r5_rproc_request_mbox(rproc);
+ if (ret)
+ return ret;
+
+ dev_err(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.
+ */
+static int k3_r5_rproc_detach(struct rproc *rproc)
+{
+ struct k3_r5_rproc *kproc = rproc->priv;
+ struct device *dev = kproc->dev;
+
+ if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
+ dev_err(dev, "R5F is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
+ return -EINVAL;
+ }
+
+ mbox_free_channel(kproc->mbox);
+ dev_err(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.
+ */
+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
*
@@ -709,8 +811,11 @@ static const struct rproc_ops k3_r5_rproc_ops = {
.unprepare = k3_r5_rproc_unprepare,
.start = k3_r5_rproc_start,
.stop = k3_r5_rproc_stop,
+ .attach = k3_r5_rproc_attach,
+ .detach = k3_r5_rproc_detach,
.kick = k3_r5_rproc_kick,
.da_to_va = k3_r5_rproc_da_to_va,
+ .get_loaded_rsc_table = k3_r5_get_loaded_rsc_table,
};

/*
@@ -1014,6 +1119,109 @@ 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_err(cdev, "configured R5F for IPC-only mode\n");
+ kproc->rproc->state = RPROC_DETACHED;
+ kproc->rproc->detach_on_shutdown = true;
+ kproc->ipc_only = true;
+ ret = 1;
+ } else if (!c_state) {
+ dev_err(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 +1262,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 +1275,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.30.1

2021-05-22 00:08:10

by Suman Anna

[permalink] [raw]
Subject: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

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 various other flags in both
remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
by a bootloader.

Signed-off-by: Suman Anna <[email protected]>
---
drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
1 file changed, 138 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
index faf60a274e8d..b154a52f1fa6 100644
--- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
@@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
* @ti_sci_id: TI-SCI device identifier
* @mbox: mailbox channel handle
* @client: mailbox client to request the mailbox channel
+ * @ipc_only: flag to indicate IPC-only mode
*/
struct k3_dsp_rproc {
struct device *dev;
@@ -91,6 +92,7 @@ struct k3_dsp_rproc {
u32 ti_sci_id;
struct mbox_chan *mbox;
struct mbox_client client;
+ bool ipc_only;
};

/**
@@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
struct device *dev = kproc->dev;
int ret;

+ /* IPC-only mode does not require the core to be released from reset */
+ if (kproc->ipc_only)
+ return 0;
+
ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
kproc->ti_sci_id);
if (ret)
@@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
struct device *dev = kproc->dev;
int ret;

+ /* do not put back the cores into reset in IPC-only mode */
+ if (kproc->ipc_only)
+ return 0;
+
ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
kproc->ti_sci_id);
if (ret)
@@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
u32 boot_addr;
int ret;

+ if (kproc->ipc_only) {
+ dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
+ __func__);
+ return -EINVAL;
+ }
+
ret = k3_dsp_rproc_request_mbox(rproc);
if (ret)
return ret;
@@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
static int k3_dsp_rproc_stop(struct rproc *rproc)
{
struct k3_dsp_rproc *kproc = rproc->priv;
+ struct device *dev = kproc->dev;
+
+ if (kproc->ipc_only) {
+ dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
+ __func__);
+ return -EINVAL;
+ }

mbox_free_channel(kproc->mbox);

@@ -359,6 +382,85 @@ 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.
+ */
+static int k3_dsp_rproc_attach(struct rproc *rproc)
+{
+ struct k3_dsp_rproc *kproc = rproc->priv;
+ struct device *dev = kproc->dev;
+ int ret;
+
+ if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
+ dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
+ return -EINVAL;
+ }
+
+ ret = k3_dsp_rproc_request_mbox(rproc);
+ if (ret)
+ return ret;
+
+ dev_err(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.
+ */
+static int k3_dsp_rproc_detach(struct rproc *rproc)
+{
+ struct k3_dsp_rproc *kproc = rproc->priv;
+ struct device *dev = kproc->dev;
+
+ if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
+ dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
+ return -EINVAL;
+ }
+
+ mbox_free_channel(kproc->mbox);
+ dev_err(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.
+ */
+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
@@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
static const struct rproc_ops k3_dsp_rproc_ops = {
.start = k3_dsp_rproc_start,
.stop = k3_dsp_rproc_stop,
+ .attach = k3_dsp_rproc_attach,
+ .detach = k3_dsp_rproc_detach,
.kick = k3_dsp_rproc_kick,
.da_to_va = k3_dsp_rproc_da_to_va,
+ .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
};

static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
@@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
struct k3_dsp_rproc *kproc;
struct rproc *rproc;
const char *fw_name;
+ bool r_state = false;
+ bool p_state = false;
int ret = 0;
int ret1;

@@ -683,19 +790,37 @@ 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,
+ &r_state, &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_err(dev, "configured DSP for IPC-only mode\n");
+ rproc->state = RPROC_DETACHED;
+ rproc->detach_on_shutdown = true;
+ kproc->ipc_only = true;
+ } else {
+ dev_err(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.30.1

2021-05-28 06:18:39

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

On Fri 21 May 19:03 CDT 2021, 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 various other flags in both
> remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
> by a bootloader.
>
> Signed-off-by: Suman Anna <[email protected]>
> ---
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
> 1 file changed, 138 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> index faf60a274e8d..b154a52f1fa6 100644
> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
> * @ti_sci_id: TI-SCI device identifier
> * @mbox: mailbox channel handle
> * @client: mailbox client to request the mailbox channel
> + * @ipc_only: flag to indicate IPC-only mode
> */
> struct k3_dsp_rproc {
> struct device *dev;
> @@ -91,6 +92,7 @@ struct k3_dsp_rproc {
> u32 ti_sci_id;
> struct mbox_chan *mbox;
> struct mbox_client client;
> + bool ipc_only;
> };
>
> /**
> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
> struct device *dev = kproc->dev;
> int ret;
>
> + /* IPC-only mode does not require the core to be released from reset */
> + if (kproc->ipc_only)

Rather than saying "prepare shouldn't do anything", how about not
actually providing prepare/unprepare ops when ipc_only?

> + return 0;
> +
> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
> kproc->ti_sci_id);
> if (ret)
> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
> struct device *dev = kproc->dev;
> int ret;
>
> + /* do not put back the cores into reset in IPC-only mode */
> + if (kproc->ipc_only)
> + return 0;
> +
> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> kproc->ti_sci_id);
> if (ret)
> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> u32 boot_addr;
> int ret;
>
> + if (kproc->ipc_only) {

It doesn't seem to make sense to start a remoteproc in ipc_only mode, so
how about not registering the start ops?

> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> ret = k3_dsp_rproc_request_mbox(rproc);
> if (ret)
> return ret;
> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> static int k3_dsp_rproc_stop(struct rproc *rproc)
> {
> struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> +
> + if (kproc->ipc_only) {
> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
>
> mbox_free_channel(kproc->mbox);
>
> @@ -359,6 +382,85 @@ 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.
> + */
> +static int k3_dsp_rproc_attach(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {

As attach only makes sense when ipc_only, how about not specifying the
attach ops when !ipc_only?

> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
> + return -EINVAL;
> + }
> +
> + ret = k3_dsp_rproc_request_mbox(rproc);
> + if (ret)
> + return ret;
> +
> + dev_err(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.
> + */
> +static int k3_dsp_rproc_detach(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {

Ditto.

> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
> + return -EINVAL;
> + }
> +
> + mbox_free_channel(kproc->mbox);
> + dev_err(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.
> + */
> +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;

Why can't you use kproc->rmem[0].size here?

> + 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
> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
> static const struct rproc_ops k3_dsp_rproc_ops = {

So in essence, I suggest that you create a separate k3_dsp_ipc_only_ops.

> .start = k3_dsp_rproc_start,
> .stop = k3_dsp_rproc_stop,
> + .attach = k3_dsp_rproc_attach,
> + .detach = k3_dsp_rproc_detach,
> .kick = k3_dsp_rproc_kick,
> .da_to_va = k3_dsp_rproc_da_to_va,
> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
> };
>
> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
> struct k3_dsp_rproc *kproc;
> struct rproc *rproc;
> const char *fw_name;
> + bool r_state = false;
> + bool p_state = false;
> int ret = 0;
> int ret1;
>
> @@ -683,19 +790,37 @@ 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,
> + &r_state, &p_state);

You can pass NULL instead of &r_state, as you don't use it anyways.

> + 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_err(dev, "configured DSP for IPC-only mode\n");

This sounds like a good thing, so perhaps dev_info() rather than err?

> + rproc->state = RPROC_DETACHED;
> + rproc->detach_on_shutdown = true;
> + kproc->ipc_only = true;
> + } else {
> + dev_err(dev, "configured DSP for remoteproc mode\n");

Ditto

Regards,
Bjorn
> + /*
> + * 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.30.1
>

2021-05-28 07:24:48

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/6] remoteproc: Add support for detach-only during shutdown

On Fri 21 May 19:03 CDT 2021, 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.
>
> Introduce a new rproc state flag 'detach_on_shutdown' that individual
> remoteproc drivers can set to only allow detach in rproc_shutdown()
> that would have been invoked when the driver is uninstalled, so that
> remote processor continues to run undisturbed even after the driver
> removal.
>

I dislike the introduction of knobs for everything and would much rather
see that we define some sound defaults. Can we make shutdown just do
detach() if that's supported otherwise stop().

This still allows userspace to explicitly stop the detachable remoteproc
before shutdown, if for some reason that's what you want...

Regards,
Bjorn

> Signed-off-by: Suman Anna <[email protected]>
> ---
> drivers/remoteproc/remoteproc_cdev.c | 7 +++++++
> drivers/remoteproc/remoteproc_core.c | 5 ++++-
> drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
> include/linux/remoteproc.h | 3 +++
> 4 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index 0b8a84c04f76..473467711a09 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->detach_on_shutdown) {
> + 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 6019f46001c8..e8ab3eb41f00 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
> if (!atomic_dec_and_test(&rproc->power))
> goto out;
>
> - ret = rproc_stop(rproc, false);
> + if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
> + 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..1785fbcb1075 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->detach_on_shutdown) {
> + 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)
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 42a1f30e33a7..35ef921676a1 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -530,6 +530,8 @@ struct rproc_dump_segment {
> * @elf_machine: firmware ELF machine
> * @cdev: character device of the rproc
> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
> + * attached state and _only_ support detach
> */
> struct rproc {
> struct list_head node;
> @@ -569,6 +571,7 @@ struct rproc {
> u16 elf_machine;
> struct cdev cdev;
> bool cdev_put_on_release;
> + bool detach_on_shutdown;
> };
>
> /**
> --
> 2.30.1
>

2021-05-28 17:19:36

by Suman Anna

[permalink] [raw]
Subject: Re: [PATCH 2/6] remoteproc: Add support for detach-only during shutdown

Hi Bjorn,

On 5/27/21 11:11 PM, Bjorn Andersson wrote:
> On Fri 21 May 19:03 CDT 2021, 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.
>>
>> Introduce a new rproc state flag 'detach_on_shutdown' that individual
>> remoteproc drivers can set to only allow detach in rproc_shutdown()
>> that would have been invoked when the driver is uninstalled, so that
>> remote processor continues to run undisturbed even after the driver
>> removal.
>>
>
> I dislike the introduction of knobs for everything and would much rather
> see that we define some sound defaults. Can we make shutdown just do
> detach() if that's supported otherwise stop().
>

I maybe missing your point, but the change in remoteproc_core below exactly does
that, right? Are you saying drop the checks in remoteproc_cdev and remoteproc_sysfs?

The asymmetry did bug me as well, but it is already existing even before this
patch. I personally would have preferred a cleaner and symmetrical attach,
start, stop, detach, but existing code has overloaded attach into start (keys
off by RPROC_OFFLINE/RPROC_DETACHED) while introducing a separate detach from
stop. I have retained the meaning of stop as shutdown from userspace interface
perspective, but enforcing the checks for detach only remoteprocs.

The logic in rproc_shutdown is for driver paths.

> This still allows userspace to explicitly stop the detachable remoteproc
> before shutdown, if for some reason that's what you want...

This is the existing behavior and the difference between stop and detach. That
behavior is maintained for remoteprocs not setting the detach_on_shutdown flag.
I am only restricting the behavior for those that set it.

Mathieu,
Your thoughts on this?

regards
Suman



>
> Regards,
> Bjorn
>
>> Signed-off-by: Suman Anna <[email protected]>
>> ---
>> drivers/remoteproc/remoteproc_cdev.c | 7 +++++++
>> drivers/remoteproc/remoteproc_core.c | 5 ++++-
>> drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>> include/linux/remoteproc.h | 3 +++
>> 4 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
>> index 0b8a84c04f76..473467711a09 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->detach_on_shutdown) {
>> + 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 6019f46001c8..e8ab3eb41f00 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
>> if (!atomic_dec_and_test(&rproc->power))
>> goto out;
>>
>> - ret = rproc_stop(rproc, false);
>> + if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
>> + 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..1785fbcb1075 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->detach_on_shutdown) {
>> + 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)
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 42a1f30e33a7..35ef921676a1 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -530,6 +530,8 @@ struct rproc_dump_segment {
>> * @elf_machine: firmware ELF machine
>> * @cdev: character device of the rproc
>> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
>> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
>> + * attached state and _only_ support detach
>> */
>> struct rproc {
>> struct list_head node;
>> @@ -569,6 +571,7 @@ struct rproc {
>> u16 elf_machine;
>> struct cdev cdev;
>> bool cdev_put_on_release;
>> + bool detach_on_shutdown;
>> };
>>
>> /**
>> --
>> 2.30.1
>>

2021-05-28 17:21:38

by Suman Anna

[permalink] [raw]
Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

Hi Bjorn,

On 5/27/21 11:36 PM, Bjorn Andersson wrote:
> On Fri 21 May 19:03 CDT 2021, 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 various other flags in both
>> remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
>> by a bootloader.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> ---
>> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
>> 1 file changed, 138 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> index faf60a274e8d..b154a52f1fa6 100644
>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
>> * @ti_sci_id: TI-SCI device identifier
>> * @mbox: mailbox channel handle
>> * @client: mailbox client to request the mailbox channel
>> + * @ipc_only: flag to indicate IPC-only mode
>> */
>> struct k3_dsp_rproc {
>> struct device *dev;
>> @@ -91,6 +92,7 @@ struct k3_dsp_rproc {
>> u32 ti_sci_id;
>> struct mbox_chan *mbox;
>> struct mbox_client client;
>> + bool ipc_only;
>> };
>>
>> /**
>> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
>> struct device *dev = kproc->dev;
>> int ret;
>>
>> + /* IPC-only mode does not require the core to be released from reset */
>> + if (kproc->ipc_only)
>
> Rather than saying "prepare shouldn't do anything", how about not
> actually providing prepare/unprepare ops when ipc_only?

I could do that but it won't provide scalability for me if I want to enhance
this to support both options, esp. given that the ops registration is a one-time
setting right now in probe.

>
>> + return 0;
>> +
>> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
>> kproc->ti_sci_id);
>> if (ret)
>> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
>> struct device *dev = kproc->dev;
>> int ret;
>>
>> + /* do not put back the cores into reset in IPC-only mode */
>> + if (kproc->ipc_only)
>> + return 0;
>> +
>> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>> kproc->ti_sci_id);
>> if (ret)
>> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>> u32 boot_addr;
>> int ret;
>>
>> + if (kproc->ipc_only) {
>
> It doesn't seem to make sense to start a remoteproc in ipc_only mode, so
> how about not registering the start ops?

Similar argument as above. This path is only to provide a sanity checking, and
would be needed if I support both options. I am not sure if changing the ops
dynamically is a better option than doing this sanity checks.

>
>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
>> + __func__);
>> + return -EINVAL;
>> + }
>> +
>> ret = k3_dsp_rproc_request_mbox(rproc);
>> if (ret)
>> return ret;
>> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>> static int k3_dsp_rproc_stop(struct rproc *rproc)
>> {
>> struct k3_dsp_rproc *kproc = rproc->priv;
>> + struct device *dev = kproc->dev;
>> +
>> + if (kproc->ipc_only) {
>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
>> + __func__);
>> + return -EINVAL;
>> + }
>>
>> mbox_free_channel(kproc->mbox);
>>
>> @@ -359,6 +382,85 @@ 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.
>> + */
>> +static int k3_dsp_rproc_attach(struct rproc *rproc)
>> +{
>> + struct k3_dsp_rproc *kproc = rproc->priv;
>> + struct device *dev = kproc->dev;
>> + int ret;
>> +
>> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
>
> As attach only makes sense when ipc_only, how about not specifying the
> attach ops when !ipc_only?

This is again used only for sanity-checking, and added for symmetry similar to
the change in start. Ideally, we should not enter this code path.

>
>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = k3_dsp_rproc_request_mbox(rproc);
>> + if (ret)
>> + return ret;
>> +
>> + dev_err(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.
>> + */
>> +static int k3_dsp_rproc_detach(struct rproc *rproc)
>> +{
>> + struct k3_dsp_rproc *kproc = rproc->priv;
>> + struct device *dev = kproc->dev;
>> +
>> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
>
> Ditto.
>
>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
>> + return -EINVAL;
>> + }
>> +
>> + mbox_free_channel(kproc->mbox);
>> + dev_err(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.
>> + */
>> +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;
>
> Why can't you use kproc->rmem[0].size here?

That's the whole reserved memory area size, which will also be used for all the
other firmware segments. ST and IMX are assigning 1K as the size, and I went
with an even smaller size.

>
>> + 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
>> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
>> static const struct rproc_ops k3_dsp_rproc_ops = {
>
> So in essence, I suggest that you create a separate k3_dsp_ipc_only_ops.
>

rproc core does a memdup, so supporting both later on would make it complex
IMHO. If you insist, I can make the changes, and probably come back to this
version when I have to support both options (my future change would just be to
provide a sysfs/configfs knob to change the detach_on_shutdown flag if using
this code).

>> .start = k3_dsp_rproc_start,
>> .stop = k3_dsp_rproc_stop,
>> + .attach = k3_dsp_rproc_attach,
>> + .detach = k3_dsp_rproc_detach,
>> .kick = k3_dsp_rproc_kick,
>> .da_to_va = k3_dsp_rproc_da_to_va,
>> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
>> };
>>
>> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
>> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>> struct k3_dsp_rproc *kproc;
>> struct rproc *rproc;
>> const char *fw_name;
>> + bool r_state = false;
>> + bool p_state = false;
>> int ret = 0;
>> int ret1;
>>
>> @@ -683,19 +790,37 @@ 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,
>> + &r_state, &p_state);
>
> You can pass NULL instead of &r_state, as you don't use it anyways.

Yeah ok. I use it on R5, but not DSP.

>
>> + 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_err(dev, "configured DSP for IPC-only mode\n");
>
> This sounds like a good thing, so perhaps dev_info() rather than err?

Yeah, will do.

>
>> + rproc->state = RPROC_DETACHED;
>> + rproc->detach_on_shutdown = true;
>> + kproc->ipc_only = true;
>> + } else {
>> + dev_err(dev, "configured DSP for remoteproc mode\n");
>
> Ditto

Will fix.

Thanks for the review. I will refresh the series and make similar changes to R5F
driver as well once I get some more feedback on the core patches (and hear from
Mathieu as well).

regards
Suman

>
> Regards,
> Bjorn
>> + /*
>> + * 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.30.1
>>

2021-06-01 17:17:09

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 2/6] remoteproc: Add support for detach-only during shutdown

Hey guys,

On Fri, May 28, 2021 at 11:40:02AM -0500, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/27/21 11:11 PM, Bjorn Andersson wrote:
> > On Fri 21 May 19:03 CDT 2021, 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.
> >>
> >> Introduce a new rproc state flag 'detach_on_shutdown' that individual
> >> remoteproc drivers can set to only allow detach in rproc_shutdown()
> >> that would have been invoked when the driver is uninstalled, so that
> >> remote processor continues to run undisturbed even after the driver
> >> removal.
> >>
> >
> > I dislike the introduction of knobs for everything and would much rather
> > see that we define some sound defaults. Can we make shutdown just do
> > detach() if that's supported otherwise stop().
> >
>
> I maybe missing your point, but the change in remoteproc_core below exactly does
> that, right? Are you saying drop the checks in remoteproc_cdev and remoteproc_sysfs?
>
> The asymmetry did bug me as well, but it is already existing even before this
> patch. I personally would have preferred a cleaner and symmetrical attach,
> start, stop, detach, but existing code has overloaded attach into start (keys
> off by RPROC_OFFLINE/RPROC_DETACHED) while introducing a separate detach from
> stop. I have retained the meaning of stop as shutdown from userspace interface
> perspective, but enforcing the checks for detach only remoteprocs.
>
> The logic in rproc_shutdown is for driver paths.
>
> > This still allows userspace to explicitly stop the detachable remoteproc
> > before shutdown, if for some reason that's what you want...
>
> This is the existing behavior and the difference between stop and detach. That
> behavior is maintained for remoteprocs not setting the detach_on_shutdown flag.
> I am only restricting the behavior for those that set it.
>
> Mathieu,
> Your thoughts on this?

Introducing knobs in such a way makes the code very difficult to understand and
maintain. It is also a matter of time before another knob is introduced to
modify the behavior of this knob.

Function rproc_detach() is exported and should be used in the platform driver if
the state of the remote processor mandates it. Function rproc_del() calls
rproc_shutdown() but the latter will return immediately because of rproc->power.
So calling rproc_detach() followed by rproc_del() will work as expected. The
real fix is to de-couple rproc_shutdown from rproc_del() and do the right calls
in the platform drivers using them.

With regards to rproc_cdev_write(), the state of the remote processor is
advertised in sysfs. As such it should be easy to write "stop" or "detach" to
the character interface. If a command to stop the remote processor is not
supported in a scenario then rproc_ops::stop should reflect that. If that is
the case then rproc_shutdown() should be modified to return an error code, the
same way rproc_detach() was done.

>
> regards
> Suman
>
>
>
> >
> > Regards,
> > Bjorn
> >
> >> Signed-off-by: Suman Anna <[email protected]>
> >> ---
> >> drivers/remoteproc/remoteproc_cdev.c | 7 +++++++
> >> drivers/remoteproc/remoteproc_core.c | 5 ++++-
> >> drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
> >> include/linux/remoteproc.h | 3 +++
> >> 4 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> >> index 0b8a84c04f76..473467711a09 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->detach_on_shutdown) {
> >> + 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 6019f46001c8..e8ab3eb41f00 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
> >> if (!atomic_dec_and_test(&rproc->power))
> >> goto out;
> >>
> >> - ret = rproc_stop(rproc, false);
> >> + if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
> >> + 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..1785fbcb1075 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->detach_on_shutdown) {
> >> + 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)
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index 42a1f30e33a7..35ef921676a1 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -530,6 +530,8 @@ struct rproc_dump_segment {
> >> * @elf_machine: firmware ELF machine
> >> * @cdev: character device of the rproc
> >> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> >> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
> >> + * attached state and _only_ support detach
> >> */
> >> struct rproc {
> >> struct list_head node;
> >> @@ -569,6 +571,7 @@ struct rproc {
> >> u16 elf_machine;
> >> struct cdev cdev;
> >> bool cdev_put_on_release;
> >> + bool detach_on_shutdown;
> >> };
> >>
> >> /**
> >> --
> >> 2.30.1
> >>
>

2021-06-01 17:53:12

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 4/6] remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs

On Fri, May 21, 2021 at 07:03:07PM -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 various other flags in both
> remoteproc core and the K3 R5F remoteproc driver. 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.
>
> Signed-off-by: Suman Anna <[email protected]>
> ---
> drivers/remoteproc/ti_k3_r5_remoteproc.c | 215 +++++++++++++++++++++++
> 1 file changed, 215 insertions(+)

Reviewed-by: Mathieu Poirier <[email protected]>

I'm out of time for today, more comments tomorrow.

>
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index e7e1ca71763e..66f0654f0860 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -151,6 +151,7 @@ struct k3_r5_core {
> * @core: cached pointer to r5 core structure being used
> * @rmem: reserved memory regions data
> * @num_rmems: number of reserved memory regions
> + * @ipc_only: flag to indicate IPC-only mode
> */
> struct k3_r5_rproc {
> struct device *dev;
> @@ -161,6 +162,7 @@ struct k3_r5_rproc {
> struct k3_r5_core *core;
> struct k3_r5_mem *rmem;
> int num_rmems;
> + bool ipc_only;
> };
>
> /**
> @@ -440,6 +442,10 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
> bool mem_init_dis;
> int ret;
>
> + /* IPC-only mode does not require the cores to be released from reset */
> + if (kproc->ipc_only)
> + return 0;
> +
> ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat);
> if (ret < 0)
> return ret;
> @@ -503,6 +509,10 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc)
> struct device *dev = kproc->dev;
> int ret;
>
> + /* do not put back the cores into reset in IPC-only mode */
> + if (kproc->ipc_only)
> + return 0;
> +
> /* Re-use LockStep-mode reset logic for Single-CPU mode */
> ret = (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
> cluster->mode == CLUSTER_MODE_SINGLECPU) ?
> @@ -538,6 +548,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> u32 boot_addr;
> int ret;
>
> + if (kproc->ipc_only) {
> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> ret = k3_r5_rproc_request_mbox(rproc);
> if (ret)
> return ret;
> @@ -604,9 +620,16 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> {
> struct k3_r5_rproc *kproc = rproc->priv;
> struct k3_r5_cluster *cluster = kproc->cluster;
> + struct device *dev = kproc->dev;
> struct k3_r5_core *core = kproc->core;
> int ret;
>
> + if (kproc->ipc_only) {
> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> /* halt all applicable cores */
> if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> list_for_each_entry(core, &cluster->cores, elem) {
> @@ -635,6 +658,85 @@ 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.
> + */
> +static int k3_r5_rproc_attach(struct rproc *rproc)
> +{
> + struct k3_r5_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
> + dev_err(dev, "R5F is expected to be in IPC-only mode and RPROC_DETACHED state\n");
> + return -EINVAL;
> + }
> +
> + ret = k3_r5_rproc_request_mbox(rproc);
> + if (ret)
> + return ret;
> +
> + dev_err(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.
> + */
> +static int k3_r5_rproc_detach(struct rproc *rproc)
> +{
> + struct k3_r5_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
> + dev_err(dev, "R5F is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
> + return -EINVAL;
> + }
> +
> + mbox_free_channel(kproc->mbox);
> + dev_err(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.
> + */
> +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
> *
> @@ -709,8 +811,11 @@ static const struct rproc_ops k3_r5_rproc_ops = {
> .unprepare = k3_r5_rproc_unprepare,
> .start = k3_r5_rproc_start,
> .stop = k3_r5_rproc_stop,
> + .attach = k3_r5_rproc_attach,
> + .detach = k3_r5_rproc_detach,
> .kick = k3_r5_rproc_kick,
> .da_to_va = k3_r5_rproc_da_to_va,
> + .get_loaded_rsc_table = k3_r5_get_loaded_rsc_table,
> };
>
> /*
> @@ -1014,6 +1119,109 @@ 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_err(cdev, "configured R5F for IPC-only mode\n");
> + kproc->rproc->state = RPROC_DETACHED;
> + kproc->rproc->detach_on_shutdown = true;
> + kproc->ipc_only = true;
> + ret = 1;
> + } else if (!c_state) {
> + dev_err(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 +1262,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 +1275,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.30.1
>

2021-06-02 15:55:13

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 4/6] remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs

I've been thinking about this patch again...

On Fri, May 21, 2021 at 07:03:07PM -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 various other flags in both
> remoteproc core and the K3 R5F remoteproc driver. 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.
>
> Signed-off-by: Suman Anna <[email protected]>
> ---
> drivers/remoteproc/ti_k3_r5_remoteproc.c | 215 +++++++++++++++++++++++
> 1 file changed, 215 insertions(+)
>
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index e7e1ca71763e..66f0654f0860 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -151,6 +151,7 @@ struct k3_r5_core {
> * @core: cached pointer to r5 core structure being used
> * @rmem: reserved memory regions data
> * @num_rmems: number of reserved memory regions
> + * @ipc_only: flag to indicate IPC-only mode
> */
> struct k3_r5_rproc {
> struct device *dev;
> @@ -161,6 +162,7 @@ struct k3_r5_rproc {
> struct k3_r5_core *core;
> struct k3_r5_mem *rmem;
> int num_rmems;
> + bool ipc_only;
> };
>
> /**
> @@ -440,6 +442,10 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
> bool mem_init_dis;
> int ret;
>
> + /* IPC-only mode does not require the cores to be released from reset */
> + if (kproc->ipc_only)
> + return 0;
> +
> ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, &stat);
> if (ret < 0)
> return ret;
> @@ -503,6 +509,10 @@ static int k3_r5_rproc_unprepare(struct rproc *rproc)
> struct device *dev = kproc->dev;
> int ret;
>
> + /* do not put back the cores into reset in IPC-only mode */
> + if (kproc->ipc_only)
> + return 0;
> +
> /* Re-use LockStep-mode reset logic for Single-CPU mode */
> ret = (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
> cluster->mode == CLUSTER_MODE_SINGLECPU) ?
> @@ -538,6 +548,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> u32 boot_addr;
> int ret;
>
> + if (kproc->ipc_only) {
> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> ret = k3_r5_rproc_request_mbox(rproc);
> if (ret)
> return ret;
> @@ -604,9 +620,16 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> {
> struct k3_r5_rproc *kproc = rproc->priv;
> struct k3_r5_cluster *cluster = kproc->cluster;
> + struct device *dev = kproc->dev;
> struct k3_r5_core *core = kproc->core;
> int ret;
>
> + if (kproc->ipc_only) {
> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
> +

It would be much cleaner to simply not provide any of the above functions to
rproc_ops when in IPC only mode.

> /* halt all applicable cores */
> if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> list_for_each_entry(core, &cluster->cores, elem) {
> @@ -635,6 +658,85 @@ 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.
> + */
> +static int k3_r5_rproc_attach(struct rproc *rproc)
> +{
> + struct k3_r5_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
> + dev_err(dev, "R5F is expected to be in IPC-only mode and RPROC_DETACHED state\n");
> + return -EINVAL;
> + }

Not sure this condition is needed since this can only be called by the core if
state == RPROC_DETACHED, same for k3_r5_rproc_detach() below. And because we are
in k3_r5_rproc_attach/detach() we can only be in IPC only mode, which means that
k3_r5_proc::ipc_only mode shouldn't be needed.

> +
> + ret = k3_r5_rproc_request_mbox(rproc);
> + if (ret)
> + return ret;
> +
> + dev_err(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.
> + */
> +static int k3_r5_rproc_detach(struct rproc *rproc)
> +{
> + struct k3_r5_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
> + dev_err(dev, "R5F is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
> + return -EINVAL;
> + }
> +
> + mbox_free_channel(kproc->mbox);
> + dev_err(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.
> + */
> +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
> *
> @@ -709,8 +811,11 @@ static const struct rproc_ops k3_r5_rproc_ops = {
> .unprepare = k3_r5_rproc_unprepare,
> .start = k3_r5_rproc_start,
> .stop = k3_r5_rproc_stop,
> + .attach = k3_r5_rproc_attach,
> + .detach = k3_r5_rproc_detach,
> .kick = k3_r5_rproc_kick,
> .da_to_va = k3_r5_rproc_da_to_va,
> + .get_loaded_rsc_table = k3_r5_get_loaded_rsc_table,
> };
>
> /*
> @@ -1014,6 +1119,109 @@ 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_err(cdev, "configured R5F for IPC-only mode\n");
> + kproc->rproc->state = RPROC_DETACHED;
> + kproc->rproc->detach_on_shutdown = true;
> + kproc->ipc_only = true;
> + ret = 1;
> + } else if (!c_state) {
> + dev_err(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 +1262,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 +1275,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.30.1
>

2021-06-02 16:09:32

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 5/6] remoteproc: k3-dsp: Refactor mbox request code in start

On Fri, May 21, 2021 at 07:03:08PM -0500, Suman Anna wrote:
> 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 <[email protected]>
> ---
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 65 ++++++++++++++---------
> 1 file changed, 39 insertions(+), 26 deletions(-)

Reviewed-by: Mathieu Poirier <[email protected]>

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

2021-06-02 16:10:46

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

On Fri, May 21, 2021 at 07:03:09PM -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 various other flags in both
> remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
> by a bootloader.
>
> Signed-off-by: Suman Anna <[email protected]>
> ---
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
> 1 file changed, 138 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> index faf60a274e8d..b154a52f1fa6 100644
> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
> * @ti_sci_id: TI-SCI device identifier
> * @mbox: mailbox channel handle
> * @client: mailbox client to request the mailbox channel
> + * @ipc_only: flag to indicate IPC-only mode
> */
> struct k3_dsp_rproc {
> struct device *dev;
> @@ -91,6 +92,7 @@ struct k3_dsp_rproc {
> u32 ti_sci_id;
> struct mbox_chan *mbox;
> struct mbox_client client;
> + bool ipc_only;
> };
>
> /**
> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
> struct device *dev = kproc->dev;
> int ret;
>
> + /* IPC-only mode does not require the core to be released from reset */
> + if (kproc->ipc_only)
> + return 0;
> +
> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
> kproc->ti_sci_id);
> if (ret)
> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
> struct device *dev = kproc->dev;
> int ret;
>
> + /* do not put back the cores into reset in IPC-only mode */
> + if (kproc->ipc_only)
> + return 0;
> +
> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> kproc->ti_sci_id);
> if (ret)
> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> u32 boot_addr;
> int ret;
>
> + if (kproc->ipc_only) {
> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> ret = k3_dsp_rproc_request_mbox(rproc);
> if (ret)
> return ret;
> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> static int k3_dsp_rproc_stop(struct rproc *rproc)
> {
> struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> +
> + if (kproc->ipc_only) {
> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> + __func__);
> + return -EINVAL;
> + }
>
> mbox_free_channel(kproc->mbox);
>
> @@ -359,6 +382,85 @@ 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.
> + */
> +static int k3_dsp_rproc_attach(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
> + return -EINVAL;
> + }
> +
> + ret = k3_dsp_rproc_request_mbox(rproc);
> + if (ret)
> + return ret;
> +
> + dev_err(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.
> + */
> +static int k3_dsp_rproc_detach(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> +
> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
> + return -EINVAL;
> + }
> +
> + mbox_free_channel(kproc->mbox);
> + dev_err(dev, "DSP deinitialized in IPC-only mode\n");
> + return 0;
> +}

Same comment as patch 4/6 regarding k3_dsp_rproc::ipc_only and setting the right
rproc_ops based on the scenario.

Thanks,
Mathieu

> +
> +/*
> + * 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.
> + */
> +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
> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
> static const struct rproc_ops k3_dsp_rproc_ops = {
> .start = k3_dsp_rproc_start,
> .stop = k3_dsp_rproc_stop,
> + .attach = k3_dsp_rproc_attach,
> + .detach = k3_dsp_rproc_detach,
> .kick = k3_dsp_rproc_kick,
> .da_to_va = k3_dsp_rproc_da_to_va,
> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
> };
>
> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
> struct k3_dsp_rproc *kproc;
> struct rproc *rproc;
> const char *fw_name;
> + bool r_state = false;
> + bool p_state = false;
> int ret = 0;
> int ret1;
>
> @@ -683,19 +790,37 @@ 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,
> + &r_state, &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_err(dev, "configured DSP for IPC-only mode\n");
> + rproc->state = RPROC_DETACHED;
> + rproc->detach_on_shutdown = true;
> + kproc->ipc_only = true;
> + } else {
> + dev_err(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.30.1
>

2021-06-03 14:59:09

by Suman Anna

[permalink] [raw]
Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

Hi Mathieu,

On 6/2/21 11:07 AM, Mathieu Poirier wrote:
> On Fri, May 21, 2021 at 07:03:09PM -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 various other flags in both
>> remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
>> by a bootloader.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> ---
>> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
>> 1 file changed, 138 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> index faf60a274e8d..b154a52f1fa6 100644
>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
>> * @ti_sci_id: TI-SCI device identifier
>> * @mbox: mailbox channel handle
>> * @client: mailbox client to request the mailbox channel
>> + * @ipc_only: flag to indicate IPC-only mode
>> */
>> struct k3_dsp_rproc {
>> struct device *dev;
>> @@ -91,6 +92,7 @@ struct k3_dsp_rproc {
>> u32 ti_sci_id;
>> struct mbox_chan *mbox;
>> struct mbox_client client;
>> + bool ipc_only;
>> };
>>
>> /**
>> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
>> struct device *dev = kproc->dev;
>> int ret;
>>
>> + /* IPC-only mode does not require the core to be released from reset */
>> + if (kproc->ipc_only)
>> + return 0;
>> +
>> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
>> kproc->ti_sci_id);
>> if (ret)
>> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
>> struct device *dev = kproc->dev;
>> int ret;
>>
>> + /* do not put back the cores into reset in IPC-only mode */
>> + if (kproc->ipc_only)
>> + return 0;
>> +
>> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>> kproc->ti_sci_id);
>> if (ret)
>> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>> u32 boot_addr;
>> int ret;
>>
>> + if (kproc->ipc_only) {
>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
>> + __func__);
>> + return -EINVAL;
>> + }
>> +
>> ret = k3_dsp_rproc_request_mbox(rproc);
>> if (ret)
>> return ret;
>> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>> static int k3_dsp_rproc_stop(struct rproc *rproc)
>> {
>> struct k3_dsp_rproc *kproc = rproc->priv;
>> + struct device *dev = kproc->dev;
>> +
>> + if (kproc->ipc_only) {
>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
>> + __func__);
>> + return -EINVAL;
>> + }
>>
>> mbox_free_channel(kproc->mbox);
>>
>> @@ -359,6 +382,85 @@ 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.
>> + */
>> +static int k3_dsp_rproc_attach(struct rproc *rproc)
>> +{
>> + struct k3_dsp_rproc *kproc = rproc->priv;
>> + struct device *dev = kproc->dev;
>> + int ret;
>> +
>> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = k3_dsp_rproc_request_mbox(rproc);
>> + if (ret)
>> + return ret;
>> +
>> + dev_err(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.
>> + */
>> +static int k3_dsp_rproc_detach(struct rproc *rproc)
>> +{
>> + struct k3_dsp_rproc *kproc = rproc->priv;
>> + struct device *dev = kproc->dev;
>> +
>> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
>> + return -EINVAL;
>> + }
>> +
>> + mbox_free_channel(kproc->mbox);
>> + dev_err(dev, "DSP deinitialized in IPC-only mode\n");
>> + return 0;
>> +}
>
> Same comment as patch 4/6 regarding k3_dsp_rproc::ipc_only and setting the right
> rproc_ops based on the scenario.

OK, I will make the switch since both you and Bjorn prefer it this way. And we
can revisit later when we want to scale it to support proper shutdown as well.
FWIW, I have given my reasons for doing it the current way in a previous
response to Bjorn for similar comments.

Note that I won't be able to define a separate ops structure but rather
overwrite the ops upon detection of IPC-only mode, since I won't be able to
detect the mode until I parse the dt and query our central system processor. The
dt parsing is all done post rproc_alloc, and I need to supply a rproc_ops first.

Btw, any comments on patch 1 which is just a cleanup patch.

regards
Suman

>
> Thanks,
> Mathieu
>
>> +
>> +/*
>> + * 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.
>> + */
>> +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
>> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
>> static const struct rproc_ops k3_dsp_rproc_ops = {
>> .start = k3_dsp_rproc_start,
>> .stop = k3_dsp_rproc_stop,
>> + .attach = k3_dsp_rproc_attach,
>> + .detach = k3_dsp_rproc_detach,
>> .kick = k3_dsp_rproc_kick,
>> .da_to_va = k3_dsp_rproc_da_to_va,
>> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
>> };
>>
>> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
>> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>> struct k3_dsp_rproc *kproc;
>> struct rproc *rproc;
>> const char *fw_name;
>> + bool r_state = false;
>> + bool p_state = false;
>> int ret = 0;
>> int ret1;
>>
>> @@ -683,19 +790,37 @@ 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,
>> + &r_state, &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_err(dev, "configured DSP for IPC-only mode\n");
>> + rproc->state = RPROC_DETACHED;
>> + rproc->detach_on_shutdown = true;
>> + kproc->ipc_only = true;
>> + } else {
>> + dev_err(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.30.1
>>

2021-06-07 16:36:32

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

On Thu, Jun 03, 2021 at 09:57:06AM -0500, Suman Anna wrote:
> Hi Mathieu,
>
> On 6/2/21 11:07 AM, Mathieu Poirier wrote:
> > On Fri, May 21, 2021 at 07:03:09PM -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 various other flags in both
> >> remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
> >> by a bootloader.
> >>
> >> Signed-off-by: Suman Anna <[email protected]>
> >> ---
> >> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
> >> 1 file changed, 138 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >> index faf60a274e8d..b154a52f1fa6 100644
> >> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
> >> * @ti_sci_id: TI-SCI device identifier
> >> * @mbox: mailbox channel handle
> >> * @client: mailbox client to request the mailbox channel
> >> + * @ipc_only: flag to indicate IPC-only mode
> >> */
> >> struct k3_dsp_rproc {
> >> struct device *dev;
> >> @@ -91,6 +92,7 @@ struct k3_dsp_rproc {
> >> u32 ti_sci_id;
> >> struct mbox_chan *mbox;
> >> struct mbox_client client;
> >> + bool ipc_only;
> >> };
> >>
> >> /**
> >> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
> >> struct device *dev = kproc->dev;
> >> int ret;
> >>
> >> + /* IPC-only mode does not require the core to be released from reset */
> >> + if (kproc->ipc_only)
> >> + return 0;
> >> +
> >> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
> >> kproc->ti_sci_id);
> >> if (ret)
> >> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
> >> struct device *dev = kproc->dev;
> >> int ret;
> >>
> >> + /* do not put back the cores into reset in IPC-only mode */
> >> + if (kproc->ipc_only)
> >> + return 0;
> >> +
> >> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> >> kproc->ti_sci_id);
> >> if (ret)
> >> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> >> u32 boot_addr;
> >> int ret;
> >>
> >> + if (kproc->ipc_only) {
> >> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> >> + __func__);
> >> + return -EINVAL;
> >> + }
> >> +
> >> ret = k3_dsp_rproc_request_mbox(rproc);
> >> if (ret)
> >> return ret;
> >> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> >> static int k3_dsp_rproc_stop(struct rproc *rproc)
> >> {
> >> struct k3_dsp_rproc *kproc = rproc->priv;
> >> + struct device *dev = kproc->dev;
> >> +
> >> + if (kproc->ipc_only) {
> >> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> >> + __func__);
> >> + return -EINVAL;
> >> + }
> >>
> >> mbox_free_channel(kproc->mbox);
> >>
> >> @@ -359,6 +382,85 @@ 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.
> >> + */
> >> +static int k3_dsp_rproc_attach(struct rproc *rproc)
> >> +{
> >> + struct k3_dsp_rproc *kproc = rproc->priv;
> >> + struct device *dev = kproc->dev;
> >> + int ret;
> >> +
> >> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
> >> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ret = k3_dsp_rproc_request_mbox(rproc);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + dev_err(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.
> >> + */
> >> +static int k3_dsp_rproc_detach(struct rproc *rproc)
> >> +{
> >> + struct k3_dsp_rproc *kproc = rproc->priv;
> >> + struct device *dev = kproc->dev;
> >> +
> >> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
> >> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + mbox_free_channel(kproc->mbox);
> >> + dev_err(dev, "DSP deinitialized in IPC-only mode\n");
> >> + return 0;
> >> +}
> >
> > Same comment as patch 4/6 regarding k3_dsp_rproc::ipc_only and setting the right
> > rproc_ops based on the scenario.
>
> OK, I will make the switch since both you and Bjorn prefer it this way. And we
> can revisit later when we want to scale it to support proper shutdown as well.
> FWIW, I have given my reasons for doing it the current way in a previous
> response to Bjorn for similar comments.
>
> Note that I won't be able to define a separate ops structure but rather
> overwrite the ops upon detection of IPC-only mode, since I won't be able to
> detect the mode until I parse the dt and query our central system processor. The
> dt parsing is all done post rproc_alloc, and I need to supply a rproc_ops first.

As far as I can tell for both R5 and DSP, rproc_alloc() could be done after the
system processor is probed. What am I missing?

>
> Btw, any comments on patch 1 which is just a cleanup patch.

That patch perplexed me. If ops->detach() is not implemented and the detach
process is allowed to carry on, how does the remote processor know the
remoteproc core is no longer available?

>
> regards
> Suman
>
> >
> > Thanks,
> > Mathieu
> >
> >> +
> >> +/*
> >> + * 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.
> >> + */
> >> +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
> >> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
> >> static const struct rproc_ops k3_dsp_rproc_ops = {
> >> .start = k3_dsp_rproc_start,
> >> .stop = k3_dsp_rproc_stop,
> >> + .attach = k3_dsp_rproc_attach,
> >> + .detach = k3_dsp_rproc_detach,
> >> .kick = k3_dsp_rproc_kick,
> >> .da_to_va = k3_dsp_rproc_da_to_va,
> >> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
> >> };
> >>
> >> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
> >> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
> >> struct k3_dsp_rproc *kproc;
> >> struct rproc *rproc;
> >> const char *fw_name;
> >> + bool r_state = false;
> >> + bool p_state = false;
> >> int ret = 0;
> >> int ret1;
> >>
> >> @@ -683,19 +790,37 @@ 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,
> >> + &r_state, &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_err(dev, "configured DSP for IPC-only mode\n");
> >> + rproc->state = RPROC_DETACHED;
> >> + rproc->detach_on_shutdown = true;
> >> + kproc->ipc_only = true;
> >> + } else {
> >> + dev_err(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.30.1
> >>
>

2021-06-16 20:26:49

by Suman Anna

[permalink] [raw]
Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

Hi Mathieu,

On 6/7/21 11:33 AM, Mathieu Poirier wrote:
> On Thu, Jun 03, 2021 at 09:57:06AM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>
>> On 6/2/21 11:07 AM, Mathieu Poirier wrote:
>>> On Fri, May 21, 2021 at 07:03:09PM -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 various other flags in both
>>>> remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
>>>> by a bootloader.
>>>>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> ---
>>>> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
>>>> 1 file changed, 138 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> index faf60a274e8d..b154a52f1fa6 100644
>>>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
>>>> * @ti_sci_id: TI-SCI device identifier
>>>> * @mbox: mailbox channel handle
>>>> * @client: mailbox client to request the mailbox channel
>>>> + * @ipc_only: flag to indicate IPC-only mode
>>>> */
>>>> struct k3_dsp_rproc {
>>>> struct device *dev;
>>>> @@ -91,6 +92,7 @@ struct k3_dsp_rproc {
>>>> u32 ti_sci_id;
>>>> struct mbox_chan *mbox;
>>>> struct mbox_client client;
>>>> + bool ipc_only;
>>>> };
>>>>
>>>> /**
>>>> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
>>>> struct device *dev = kproc->dev;
>>>> int ret;
>>>>
>>>> + /* IPC-only mode does not require the core to be released from reset */
>>>> + if (kproc->ipc_only)
>>>> + return 0;
>>>> +
>>>> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
>>>> kproc->ti_sci_id);
>>>> if (ret)
>>>> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
>>>> struct device *dev = kproc->dev;
>>>> int ret;
>>>>
>>>> + /* do not put back the cores into reset in IPC-only mode */
>>>> + if (kproc->ipc_only)
>>>> + return 0;
>>>> +
>>>> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>>> kproc->ti_sci_id);
>>>> if (ret)
>>>> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>>>> u32 boot_addr;
>>>> int ret;
>>>>
>>>> + if (kproc->ipc_only) {
>>>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
>>>> + __func__);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> ret = k3_dsp_rproc_request_mbox(rproc);
>>>> if (ret)
>>>> return ret;
>>>> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>>>> static int k3_dsp_rproc_stop(struct rproc *rproc)
>>>> {
>>>> struct k3_dsp_rproc *kproc = rproc->priv;
>>>> + struct device *dev = kproc->dev;
>>>> +
>>>> + if (kproc->ipc_only) {
>>>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
>>>> + __func__);
>>>> + return -EINVAL;
>>>> + }
>>>>
>>>> mbox_free_channel(kproc->mbox);
>>>>
>>>> @@ -359,6 +382,85 @@ 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.
>>>> + */
>>>> +static int k3_dsp_rproc_attach(struct rproc *rproc)
>>>> +{
>>>> + struct k3_dsp_rproc *kproc = rproc->priv;
>>>> + struct device *dev = kproc->dev;
>>>> + int ret;
>>>> +
>>>> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
>>>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = k3_dsp_rproc_request_mbox(rproc);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + dev_err(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.
>>>> + */
>>>> +static int k3_dsp_rproc_detach(struct rproc *rproc)
>>>> +{
>>>> + struct k3_dsp_rproc *kproc = rproc->priv;
>>>> + struct device *dev = kproc->dev;
>>>> +
>>>> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
>>>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + mbox_free_channel(kproc->mbox);
>>>> + dev_err(dev, "DSP deinitialized in IPC-only mode\n");
>>>> + return 0;
>>>> +}
>>>
>>> Same comment as patch 4/6 regarding k3_dsp_rproc::ipc_only and setting the right
>>> rproc_ops based on the scenario.
>>
>> OK, I will make the switch since both you and Bjorn prefer it this way. And we
>> can revisit later when we want to scale it to support proper shutdown as well.
>> FWIW, I have given my reasons for doing it the current way in a previous
>> response to Bjorn for similar comments.
>>
>> Note that I won't be able to define a separate ops structure but rather
>> overwrite the ops upon detection of IPC-only mode, since I won't be able to
>> detect the mode until I parse the dt and query our central system processor. The
>> dt parsing is all done post rproc_alloc, and I need to supply a rproc_ops first.
>
> As far as I can tell for both R5 and DSP, rproc_alloc() could be done after the
> system processor is probed. What am I missing?

The detection requires parsing the various ti,sci properties, and all the R5F OF
properties are parsed and directly stored in the driver-specific remoteproc
private structure that is allocated as part of rproc_alloc(). Moving the
rproc_alloc() for later just for the sake of supplying different ops just makes
the whole probe function cumbersome. We already do dynamic plugging of ops
between C66x and C71x DSPs in the ti_k3_dsp_remoteproc driver, so simplest would
be to follow the same approach here.

>>
>> Btw, any comments on patch 1 which is just a cleanup patch.
>
> That patch perplexed me. If ops->detach() is not implemented and the detach
> process is allowed to carry on, how does the remote processor know the
> remoteproc core is no longer available?

See my response to Bjorn's comments on Patch 1. I am merely adding the wrapper
similar to rproc_attach_device(). If the point is that we are using the wrong
default return value, then I can fix that up and we ought to fix the same in
rproc_attach_device() as well.

regards
Suman


>
>>
>> regards
>> Suman
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>> +
>>>> +/*
>>>> + * 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.
>>>> + */
>>>> +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
>>>> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
>>>> static const struct rproc_ops k3_dsp_rproc_ops = {
>>>> .start = k3_dsp_rproc_start,
>>>> .stop = k3_dsp_rproc_stop,
>>>> + .attach = k3_dsp_rproc_attach,
>>>> + .detach = k3_dsp_rproc_detach,
>>>> .kick = k3_dsp_rproc_kick,
>>>> .da_to_va = k3_dsp_rproc_da_to_va,
>>>> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
>>>> };
>>>>
>>>> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
>>>> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>>>> struct k3_dsp_rproc *kproc;
>>>> struct rproc *rproc;
>>>> const char *fw_name;
>>>> + bool r_state = false;
>>>> + bool p_state = false;
>>>> int ret = 0;
>>>> int ret1;
>>>>
>>>> @@ -683,19 +790,37 @@ 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,
>>>> + &r_state, &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_err(dev, "configured DSP for IPC-only mode\n");
>>>> + rproc->state = RPROC_DETACHED;
>>>> + rproc->detach_on_shutdown = true;
>>>> + kproc->ipc_only = true;
>>>> + } else {
>>>> + dev_err(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.30.1
>>>>
>>

2021-06-22 22:52:50

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs

Good day Suman,

On Wed, Jun 16, 2021 at 10:00:42AM -0500, Suman Anna wrote:
> Hi Mathieu,
>
> On 6/7/21 11:33 AM, Mathieu Poirier wrote:
> > On Thu, Jun 03, 2021 at 09:57:06AM -0500, Suman Anna wrote:
> >> Hi Mathieu,
> >>
> >> On 6/2/21 11:07 AM, Mathieu Poirier wrote:
> >>> On Fri, May 21, 2021 at 07:03:09PM -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 various other flags in both
> >>>> remoteproc core and the K3 DSP remoteproc driver. 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 R5F core has been started
> >>>> by a bootloader.
> >>>>
> >>>> Signed-off-by: Suman Anna <[email protected]>
> >>>> ---
> >>>> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++--
> >>>> 1 file changed, 138 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >>>> index faf60a274e8d..b154a52f1fa6 100644
> >>>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >>>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >>>> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data {
> >>>> * @ti_sci_id: TI-SCI device identifier
> >>>> * @mbox: mailbox channel handle
> >>>> * @client: mailbox client to request the mailbox channel
> >>>> + * @ipc_only: flag to indicate IPC-only mode
> >>>> */
> >>>> struct k3_dsp_rproc {
> >>>> struct device *dev;
> >>>> @@ -91,6 +92,7 @@ struct k3_dsp_rproc {
> >>>> u32 ti_sci_id;
> >>>> struct mbox_chan *mbox;
> >>>> struct mbox_client client;
> >>>> + bool ipc_only;
> >>>> };
> >>>>
> >>>> /**
> >>>> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc)
> >>>> struct device *dev = kproc->dev;
> >>>> int ret;
> >>>>
> >>>> + /* IPC-only mode does not require the core to be released from reset */
> >>>> + if (kproc->ipc_only)
> >>>> + return 0;
> >>>> +
> >>>> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
> >>>> kproc->ti_sci_id);
> >>>> if (ret)
> >>>> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc)
> >>>> struct device *dev = kproc->dev;
> >>>> int ret;
> >>>>
> >>>> + /* do not put back the cores into reset in IPC-only mode */
> >>>> + if (kproc->ipc_only)
> >>>> + return 0;
> >>>> +
> >>>> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> >>>> kproc->ti_sci_id);
> >>>> if (ret)
> >>>> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> >>>> u32 boot_addr;
> >>>> int ret;
> >>>>
> >>>> + if (kproc->ipc_only) {
> >>>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> >>>> + __func__);
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> ret = k3_dsp_rproc_request_mbox(rproc);
> >>>> if (ret)
> >>>> return ret;
> >>>> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
> >>>> static int k3_dsp_rproc_stop(struct rproc *rproc)
> >>>> {
> >>>> struct k3_dsp_rproc *kproc = rproc->priv;
> >>>> + struct device *dev = kproc->dev;
> >>>> +
> >>>> + if (kproc->ipc_only) {
> >>>> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n",
> >>>> + __func__);
> >>>> + return -EINVAL;
> >>>> + }
> >>>>
> >>>> mbox_free_channel(kproc->mbox);
> >>>>
> >>>> @@ -359,6 +382,85 @@ 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.
> >>>> + */
> >>>> +static int k3_dsp_rproc_attach(struct rproc *rproc)
> >>>> +{
> >>>> + struct k3_dsp_rproc *kproc = rproc->priv;
> >>>> + struct device *dev = kproc->dev;
> >>>> + int ret;
> >>>> +
> >>>> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) {
> >>>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n");
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> + ret = k3_dsp_rproc_request_mbox(rproc);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + dev_err(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.
> >>>> + */
> >>>> +static int k3_dsp_rproc_detach(struct rproc *rproc)
> >>>> +{
> >>>> + struct k3_dsp_rproc *kproc = rproc->priv;
> >>>> + struct device *dev = kproc->dev;
> >>>> +
> >>>> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) {
> >>>> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n");
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> + mbox_free_channel(kproc->mbox);
> >>>> + dev_err(dev, "DSP deinitialized in IPC-only mode\n");
> >>>> + return 0;
> >>>> +}
> >>>
> >>> Same comment as patch 4/6 regarding k3_dsp_rproc::ipc_only and setting the right
> >>> rproc_ops based on the scenario.
> >>
> >> OK, I will make the switch since both you and Bjorn prefer it this way. And we
> >> can revisit later when we want to scale it to support proper shutdown as well.
> >> FWIW, I have given my reasons for doing it the current way in a previous
> >> response to Bjorn for similar comments.
> >>
> >> Note that I won't be able to define a separate ops structure but rather
> >> overwrite the ops upon detection of IPC-only mode, since I won't be able to
> >> detect the mode until I parse the dt and query our central system processor. The
> >> dt parsing is all done post rproc_alloc, and I need to supply a rproc_ops first.
> >
> > As far as I can tell for both R5 and DSP, rproc_alloc() could be done after the
> > system processor is probed. What am I missing?
>
> The detection requires parsing the various ti,sci properties, and all the R5F OF
> properties are parsed and directly stored in the driver-specific remoteproc
> private structure that is allocated as part of rproc_alloc(). Moving the
> rproc_alloc() for later just for the sake of supplying different ops just makes
> the whole probe function cumbersome. We already do dynamic plugging of ops
> between C66x and C71x DSPs in the ti_k3_dsp_remoteproc driver, so simplest would
> be to follow the same approach here.
>

The real reason has nothing to do with parsing of the DT but where and how the
rproc->priv is used. And that part I agree with you - it would take a serious
refactoring exercise to change all this and it is not worth it. So yes, please
proceed the same way you did for the prepare/unprepare functions of C66x and
C71x. For the latter it would be nice to fixup all the rproc_ops in one place.

> >>
> >> Btw, any comments on patch 1 which is just a cleanup patch.
> >
> > That patch perplexed me. If ops->detach() is not implemented and the detach
> > process is allowed to carry on, how does the remote processor know the
> > remoteproc core is no longer available?
>
> See my response to Bjorn's comments on Patch 1. I am merely adding the wrapper
> similar to rproc_attach_device(). If the point is that we are using the wrong
> default return value, then I can fix that up and we ought to fix the same in
> rproc_attach_device() as well.

At this time __rproc_detach() will prevent the wrapper code from being executed,
and that is why I did not introduce a rproc_detach_device() to balance
rproc_attach_device(). An automated tool will recognised the condition and a
patch will be in my Inbox with a week.

For the i.MX folks not being able to detach from a remote process is a valid
use case.

And if rproc->ops->detach() does exist then we'll check the pointer twice.

Thanks,
Mathieu

>
> regards
> Suman
>
>
> >
> >>
> >> regards
> >> Suman
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>> +
> >>>> +/*
> >>>> + * 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.
> >>>> + */
> >>>> +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
> >>>> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
> >>>> static const struct rproc_ops k3_dsp_rproc_ops = {
> >>>> .start = k3_dsp_rproc_start,
> >>>> .stop = k3_dsp_rproc_stop,
> >>>> + .attach = k3_dsp_rproc_attach,
> >>>> + .detach = k3_dsp_rproc_detach,
> >>>> .kick = k3_dsp_rproc_kick,
> >>>> .da_to_va = k3_dsp_rproc_da_to_va,
> >>>> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table,
> >>>> };
> >>>>
> >>>> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
> >>>> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
> >>>> struct k3_dsp_rproc *kproc;
> >>>> struct rproc *rproc;
> >>>> const char *fw_name;
> >>>> + bool r_state = false;
> >>>> + bool p_state = false;
> >>>> int ret = 0;
> >>>> int ret1;
> >>>>
> >>>> @@ -683,19 +790,37 @@ 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,
> >>>> + &r_state, &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_err(dev, "configured DSP for IPC-only mode\n");
> >>>> + rproc->state = RPROC_DETACHED;
> >>>> + rproc->detach_on_shutdown = true;
> >>>> + kproc->ipc_only = true;
> >>>> + } else {
> >>>> + dev_err(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.30.1
> >>>>
> >>
>