Re: SPARC Leon baremetal layer

[Konrad Eisele <konrad at gaisler dot com>]

Mike Frysinger wrote:
> On Monday 28 November 2011 06:08:10 Konrad Eisele wrote:
>> I would like to a patch that add a baremetal layer to newlibc-1.19.0 for the SPARC-v8 variant LEON (www.gaisler.com). Is it possible to get a review so that I can streamline the patch to be added to the newlibc repository?
> 
>> --- a/libgloss/configure.in +++ b/libgloss/configure.in @@ -52,6 +52,9 @@ case "${target}" in i960-*-coff) AC_CONFIG_SUBDIRS([i960]) ;; +  sparc-*leon*-elf* | sparc-*leon*-none**) +   AC_CONFIG_SUBDIRS([sparc_leon]) +   ;; sparclet-*-aout* | sparc-*-elf* | sparc64-*-elf* | sparc86x-*-* |
> sparclite-*-*)
>> AC_CONFIG_SUBDIRS([sparc]) ;;
> 
> gcc uses "sparc-leon*-*" ... maybe this should use the same style ?

I am using the vendor field to select the various leon variants.
The variants are sfleon (soft-float) sfleonv8 (soft-float+v8)
hfleon (hard-float) hfleonv8 (hard-float+v8). Therefore I'm using
*leon*

> 
>> --- /dev/null +++ b/libgloss/link.c
>> 
>> +int +_DEFUN (_link, (existing, new), +        char *existing _AND +        char *new) +{ +  errno = EIO; +  return (-1); +}
> 
> this looks like it belongs in libgloss/libnosys/
> 
> it should also be "return -1;"

I'll remove it.

> 
>> +
> 
> please no trailing newlines in files

Ok.

> 
>> --- /dev/null +++ b/libgloss/sparc_leon/amba.c
> 
> does libgloss have a standard for sub-arches ?  my reaction would be that this should be sparc/leon/, but it seems the newlib standard is for sub-arches to be in the same level as the overall ISA.
> 
>> +/* Structure containing address to devices found on the Amba Plug&Play bus
> */
> 
> looks like a lot of these comments need converting to the GNU style.  so this should have a period at the end, and two spaces after that ...


Ok.

> 
>> +#define amba_insert_device(tab, address) \ +{ \ +  if (LEON3_BYPASS_LOAD_PA(address)) \ +  { \ +    (tab)->addr[(tab)->devnr] = (address); \ +    (tab)->devnr ++; \ +  } \ +} while(0)
> 
> seems to have more GNU style issues.  also, is this correct ?  shouldn't there be a "do" before that first opening brace ?

I'll check this. "do" might be missing.

> 
>> +void amba_init(void) +{ +   unsigned int *cfg_area; /* address to configuration area */
> 
> GNU style issues: this should be spacing for initial indentation; not tabs

I'll switch style.

> 
>> +    Copyright (C) 2004  Gaisler Research AB
> 
> does libgloss require copyright assignment to the FSF ?

I can add any copyright statement . Jiri Gaisler (CTO) has signed a
copyleft form when we submitted patches to gcc. I'm not shure weather
his applies for newlibc also.
What kind of statement should I replace it with?

> 
> i imagine these issues are going to apply to the entire code base ... -mike