Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 1 Feb 2021 17:49:56 -0700
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Arnaud POULIQUEN <arnaud.pouliquen@st.com>
Cc:     "ohad@wizery.com" <ohad@wizery.com>,
        "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 00/17] remoteproc: Add support for detaching a rproc
Message-ID: <20210202004956.GD1319650@xps15>
References: <20201218173228.2277032-1-mathieu.poirier@linaro.org>
 <64b559dc-9e89-c351-ddee-f9cebd155ed7@st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <64b559dc-9e89-c351-ddee-f9cebd155ed7@st.com>
Precedence: bulk

On Wed, Jan 27, 2021 at 10:21:24AM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu
> 
> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> > Following the work done here [1], this set provides support for the
> > remoteproc core to release resources associated with a remote processor
> > without having to switch it off. That way a platform driver can be removed
> > or the application processor power cycled while the remote processor is
> > still operating.
> > 
> > Of special interest in this series are patches 5 and 6 where getting the
> > address of the resource table installed by an eternal entity if moved to
> > the core.  This is to support scenarios where a remote process has been
> > booted by the core but is being detached.  To re-attach the remote
> > processor, the address of the resource table needs to be known at a later
> > time than the platform driver's probe() function.
> > 
> > Applies cleanly on v5.10
> > 
> > Thanks,
> > Mathieu
> > 
> > [1]. https://lkml.org/lkml/2020/7/14/1600
> > 
> > ----
> > New for v4:
> > - Made binding description OS agnostic (Rob)
> > - Added functionality to set the external resource table in the core
> > - Fixed a crash when detaching (Arnaud)
> > - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
> > - Added RB tags
> 
> 
> I tested you series, attach and  detach is working well.
> 
> Then I faced issue when tried to re-attach after a detach.
>

Right, in this case don't expect the re-attach to work properly because function
stm32_rproc_detach() does not exist.  As such the M4 doesn't put itself back
in "wait-for-attach" mode as it does when booted by the boot loader.  If I
remember correctly we talked about that during an earlier conversation and we
agreed FW support would be needed to properly test the re-attach.
 
> But I don't know if this feature has to be supported in this step.
> 
> The 2 issues I found are:
> 
> 1) memory carveouts are released on detach so need to be reinitialized.
> The use of prepare/unprepare for the attach and detach would solve the issue but
> probably need to add parameter to differentiate a start/stop from a attach/detach.
> 
> 2) The vrings in the loaded resource table (so no cached) has to be properly
> reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
> considered as a fixed address.
> 
> Here is a fix which works on the stm32 platform
> 
> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
>  	 */
>  	if (rproc->table_ptr) {
>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> -		rsc->vring[idx].da = 0;
> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
>  		rsc->vring[idx].notifyid = -1;
>  	}
>  }

In light of the above let me know if these two issues are still relevant.  If
so I'll investigate further.

Thanks,
Mathieu

> 
> Here, perhaps a better alternative would be to make a cached copy on attach
> before updating it. On the next attach, the cached copy would be copied as it is
> done in rproc_start.
> 
> Thanks,
> Arnaud
> 
> 
> > 
> > Mathieu Poirier (17):
> >   dt-bindings: remoteproc: Add bindind to support autonomous processors
> >   remoteproc: Re-check state in rproc_shutdown()
> >   remoteproc: Remove useless check in rproc_del()
> >   remoteproc: Rename function rproc_actuate()
> >   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
> >   remoteproc: stm32: Move resource table setup to rproc_ops
> >   remoteproc: Add new RPROC_ATTACHED state
> >   remoteproc: Properly represent the attached state
> >   remoteproc: Properly deal with a kernel panic when attached
> >   remoteproc: Add new detach() remoteproc operation
> >   remoteproc: Introduce function __rproc_detach()
> >   remoteproc: Introduce function rproc_detach()
> >   remoteproc: Add return value to function rproc_shutdown()
> >   remoteproc: Properly deal with a stop request when attached
> >   remoteproc: Properly deal with a start request when attached
> >   remoteproc: Properly deal with detach request
> >   remoteproc: Refactor rproc delete and cdev release path
> > 
> >  .../bindings/remoteproc/remoteproc-core.yaml  |  27 +++
> >  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
> >  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
> >  drivers/remoteproc/remoteproc_internal.h      |   8 +
> >  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
> >  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
> >  include/linux/remoteproc.h                    |  24 +-
> >  7 files changed, 344 insertions(+), 125 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> >