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

[Dimitar Dimitrov <dimitar@dinux.eu>]

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