Re: [PATCH 04/16] remoteproc/pru: Add PRU remoteproc driver

[Dimitar Dimitrov]

On Fri, 12/14 2018 11:53:56 EET Roger Quadros wrote:
> Hi Dimitar,
> 
> On 30/11/18 23:39, Dimitar Dimitrov wrote:
> > On Monday, 12/26/2018, 9:52:37 EET Roger Quadros wrote:
> >> +/*
> >> + * Convert PRU device address (instruction space) to kernel virtual
> >> address + *
> >> + * A PRU does not have an unified address space. Each PRU has its very
> >> own
> >> + * private Instruction RAM, and its device address is identical to that
> >> of
> >> + * its primary Data RAM device address.
> >> + */
> >> +static void *pru_i_da_to_va(struct pru_rproc *pru, u32 da, int len)
> >> +{
> >> +       u32 offset;
> >> +       void *va = NULL;
> >> +
> >> +       if (len <= 0)
> >> +               return NULL;
> > 
> > Could you please clear the upper 4 bits the of IRAM device address, in
> > order to support binutils ELF images? Here is an example line to add
> > here:
> > 
> > +       /* GNU binutils do not support multiple address spaces. The
> > +        * default linker script from the official GNU pru-ld places
> > +        * IRAM at an arbitrary high offset, in order to differentiate it
> > +        * from DRAM. Hence we need to strip the artificial offset
> > +        * from the IRAM address.
> > +        */
> > +       da &= ~0xf0000000u;
> > +
> 
> After some more thought I'm not very sure how to proceed.
> 
> I'll be using the below 2 patches in the next patch spin in place of
> patch 1 in the current series.
> https://lore.kernel.org/lkml/20180623210810.21232-2-david@xxxxxxxxxxxxxx/
> https://lore.kernel.org/lkml/20180623210810.21232-3-david@xxxxxxxxxxxxxx/
> 
> They figure out the PAGE (IRAM vs DRAM) by looking at TI specific section
> attributes.
> 
> e.g.
> [18] .TI.phattrs LOPROC+f000004 00000000 000e08 000010 00 0 0 4
> [19] .TI.section.flags LOPROC+f000005 00000000 000e68 00002a 00 0 0 0
> [20] .TI.section.page LOPROC+f000007 00000000 000e92 00002a 00 0 0 0
> 
> AFAIK the ELF by GNU pru-ld won't contain these sections.
These sections are not documented, so pru-ld does not generate them.

> 
> We need to support ELF generated by both tools (TI clpru and GNU pru-ld).
> Is it safe to assume that if the ELF doesn't have the TI specific sections
> then it was generated by gnupru?
Right now this is correct. My concern is that in the future TI may decide to 
document these sections, and pru-ld default linker script may get updated. On 
the other hand, if TI considers these specific to their toolchain and not 
applicable to GNU, then go ahead and use this indicator.

Slightly more robust might be to check if MSB byte of the entry address is 
non-zero:
  Entry point address:               0x20000000
                                       ^^
> 
> Is there a more straight forward way of differentiating the two. e.g. by
> looking at something in the ELF header?

Is there a reason to differentiate? As long as you zero the MSB byte of IRAM 
device addresses, there should be no conflict. Very large IRAM addresses are 
unlikely to ever be valid, so it should be safe to zero the MSB for any PRU 
ELF image.


Regards,
Dimitar