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

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

[Dimitar Dimitrov]

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
+
 #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;
 
+	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

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

[Bjorn Andersson]

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
> 

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

[Dimitar Dimitrov]

On Mon, 28 Dec 2020 19:45:41 EET Bjorn Andersson wrote:
> 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?
The 16-bit word-address range specified by PRU architecture corresponds to 18-
bit byte-address range in ELF.

I added two more bits just in case TI decides to implement banking to expand 
the IRAM in future SoCs.

I'll put this clarification in the comment.

> 
> > +
> > 
> >  #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.
Will do.

Thanks,
Dimitar