Re: [PATCH 1/2] remoteproc: drop memset when loading elf segments

[ClÃment Leger]

----- On 21 Apr, 2020, at 20:25, s-anna s-anna@xxxxxx wrote:

>>>
>>> Hi Peng,
>>>
>>> The trick is to use the ioremap_wc() variant instead of ioremap() in your
>>> platform driver while mapping the TCMs. I know multiple folks have run into
>>> this issue. This is what most of the remoteproc drivers use, and mmio-sram
>>> driver also uses the same.
>> 
>> TCM is different from OCRAM in i.MX chips.
>> ioremap_wc not work for TCM memory, I just tried that.
> 
> What ARM core is this? Is it a standard ARM TCM memory? The TCM
> interfaces from standard ARM cores in general are treated as normal
> memory, so write combine should be ok on them. When you say it doesn't
> work, whats not working - the memcpy/memset or the remoteproc doesn't boot?
> 
>> I am thinking we change memset/memcpy to use
>> memset_io/memcpy_fromio/memcpy_toio for remoteproc common code,
> 
> This has other implications. Not everything is IO memory to make this
> change in the common core.

Hi Suman,

AFAIK, most (if not all) the drivers are using ioremap to expose the
remoteproc memories. Since these are IO memories, then care should be
taken when writing/reading from them and memset_io/memcpy_fromio
/memcpy_toio should be used (or ioread/write). Currently, this is not
the case in the remoteproc driver and it works because everything is
aligned and there is no misaligned access. IMHO, as suggested by Peng,
remoteproc core should use such accessors.

We (at Kalray) have some modifications to do that since
there were some misalignment after modifying the resource table (64 bits
fields).

The only drawback that I can see using io memcpy is potentially a loss
of performance for processors not requiring any alignement check. But
other than that, it seems safer for everyon and performance in init does
not seems really critical.

> 
> If this is a custom memory interface requiring specific handling, then
> one option is to provide and use your own ops->load function within the
> driver. This is what I do for one of our remoteprocs that requires a
> specific memcpy handling semantics.

This does not only apply to firmware loading. This applies to all the
resource table accesses which are currently done using direct accesses
to members. They should also used io memcpy.

Regards,

ClÃment

> 
> regards
> Suman