RE: [PATCH v4 01/17] remoteproc: configure IOMMU only if device address requested

[Loic PALLARDY]

Hi Suman,

> -----Original Message-----
> From: Suman Anna <s-anna@xxxxxx>
> Sent: mardi 23 octobre 2018 19:26
> To: Loic PALLARDY <loic.pallardy@xxxxxx>; bjorn.andersson@xxxxxxxxxx;
> ohad@xxxxxxxxxx
> Cc: linux-remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx>;
> benjamin.gaignard@xxxxxxxxxx
> Subject: Re: [PATCH v4 01/17] remoteproc: configure IOMMU only if device
> address requested
> 
> Hi Loic,
> 
> On 7/27/18 8:14 AM, Loic Pallardy wrote:
> > If there is no IOMMU associate to remote processor device,
> > remoteproc_core won't be able to satisfy device address requested
> > in firmware resource table.
> > Return an error as configuration won't be coherent.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
> 
> This patch is breaking my Davinci platforms. It is not really required
> that you _should_ have IOMMUs when a valid DA is mentioned. Please see
> the existing description (paras 4 and 5) on the fw_rsc_carveout
> kerneldoc in remoteproc.h file.

Thanks for pointing this comment. Indeed sMMU is not mandatory, and at first sight I agree we should remove the restriction introduced by the patch.
Driver porting on the series should be done before adding this.
> 
> We do have platforms where we have some internal sub-modules within the
> remote processor sub-system that provides some linear
> address-translation (most common case with 32-bit processors supporting
> 64-bit addresses). Also, we have some upcoming SoCs where we have an
> MMU
> but is not programmable by Linux.
> 
> There is one comment there, but I don't think this is actually handled
> in the current remoteproc core.
> "If @da is set to
>  * FW_RSC_ADDR_ANY, then the host will dynamically allocate it, and then
>  * overwrite @da with the dynamically allocated address."
> 
I don't remember it was implemented like described.

I have remarks about the comment:
"* We will always use @da to negotiate the device addresses, even if it
 * isn't using an iommu. In that case, though, it will obviously contain
 * physical addresses."

When there is no sMMU, we can't consider that da contains a physical address because coprocessor can have its own memory map just because it is a 32bit processor accessing only a part of the memory and the main is 64bit one. The 2 processors won't see the internal memory at the same base address for example.

So what should we do when carveout allocated by host is not fitting with resource table request?
- put a warning and overwrite da address in the resource table?
- stop rproc probe as no match detected?

Later in the series, carveout allocation is changed. Resource table carveout are either linked with an existing carveout registered by driver or added to carveout list for allocations.
In the case you described, TI driver should first register the specific carveout regions thank to the helper.

Regards,
Loic

> regards
> Suman
> 
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 4cd1a8e..437fabf 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -657,7 +657,15 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  	 * to use the iommu-based DMA API: we expect 'dma' to contain the
> >  	 * physical address in this case.
> >  	 */
> > -	if (rproc->domain) {
> > +
> > +	if (rsc->da != FW_RSC_ADDR_ANY && !rproc->domain) {
> > +		dev_err(dev->parent,
> > +			"Bad carveout rsc configuration\n");
> > +		ret = -ENOMEM;
> > +		goto dma_free;
> > +	}
> > +
> > +	if (rsc->da != FW_RSC_ADDR_ANY && rproc->domain) {
> >  		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> >  		if (!mapping) {
> >  			ret = -ENOMEM;
> >