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 <s-anna@xxxxxx>
> >>>> ---
> >>>> 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
> >>>>
> >>
>
