Re: [PATCH] remoteproc: pru: Fix loading of GNU Binutils ELF

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 28 Dec 01:49 CST 2020, Dimitar Dimitrov wrote:

> PRU port of GNU Binutils lacks support for separate address spaces.
> PRU IRAM addresses are marked with artificial offset to differentiate
> them from DRAM addresses. Hence remoteproc must mask IRAM addresses
> coming from GNU ELF in order to get the true hardware address.
> 
> Patch was tested on top of latest linux-remoteproc/for-next branch:
>   commit 4c0943255805 ("Merge branches 'hwspinlock-next', 'rpmsg-next' and 'rproc-next' into for-next")'
> 
> PRU firmware used for testing was the example in:
>   https://github.com/dinuxbg/pru-gcc-examples/tree/master/blinking-led/pru
> 
> Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> ---
>  drivers/remoteproc/pru_rproc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index 2667919d76b3..b03114bbb9ab 100644
> --- a/drivers/remoteproc/pru_rproc.c
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -61,6 +61,18 @@
>  #define PRU_SDRAM_DA	0x2000	/* Secondary Data RAM */
>  #define PRU_SHRDRAM_DA	0x10000 /* Shared Data RAM */
>  
> +/*
> + * GNU binutils do not support multiple address spaces. The GNU linker's
> + * default linker script places IRAM at an arbitrary high offset, in order
> + * to differentiate it from DRAM. Hence we need to strip the artificial offset
> + * in the IRAM addresses coming from the ELF file.
> + *
> + * The TI proprietary linker would never set those higher IRAM address bits
> + * anyway. PRU architecture limits the program counter to 16 bit word
> + * addresses.
> + */
> +#define PRU_IRAM_DA_MASK	0xfffff

If the limit is 16 bits, why is your mask 20 bits?

> +
>  #define MAX_PRU_SYS_EVENTS 160
>  
>  /**
> @@ -450,6 +462,8 @@ static void *pru_i_da_to_va(struct pru_rproc *pru, u32 da, size_t len)
>  	if (len == 0)
>  		return NULL;
>  

Given that the comment explains this operation I think it would be
better to place it here. And if the masking directly follows what's
described in the comment you don't need a define for the mask.

Regards,
Bjorn

> +	da &= PRU_IRAM_DA_MASK;
> +
>  	if (da >= PRU_IRAM_DA &&
>  	    da + len <= PRU_IRAM_DA + pru->mem_regions[PRU_IOMEM_IRAM].size) {
>  		offset = da - PRU_IRAM_DA;
> -- 
> 2.20.1
>