[U-Boot] [PATCH 04/17] LEON3: reimplemented AMBA Plug&Play scanning routines.

[Wolfgang Denk]

Dear Daniel Hellstrom,

In message <1274194143-8994-3-git-send-email-daniel at gaisler.com> you wrote:
> Signed-off-by: Daniel Hellstrom <daniel at gaisler.com>
...
> - * (C) Copyright 2007
> - * Daniel Hellstrom, Gaisler Research, daniel at gaisler.com
> + * (C) Copyright 2010
> + * Daniel Hellstrom, Aeroflex Gaisler, daniel at gaisler.com.

Thsi cannot be. Make this "Copyright 2007, 2010" or "Copyright
2007-2010".

>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>   * MA 02111-1307 USA
> + *

Please drop this empty line.

> +/* #define DEBUG */
> +

Please do not add dead code.

...
> +	int i;
> +
> +	ambapp_find_buses(ioarea, abus);
> +	for(i=0; i<6; i++)
> +		if ( abus->ioareas[i] == 0 )
> +			break;

Multiline if's require braces. Please also fix white space:

	for (i=0; i<6; i++) {
		if (abus->ioareas[i] == 0)
			break;
	}

> +		if ( type == AMBA_TYPE_AHBIO ) {

Please fix white spaces as above. Please fix this globally.

> +	if ( found == 1 ) {
> +		ambapp_apb_parse(&apbdev, dev);
> +	}

No braces for one liners. And fix the white space.

...
> +	found = ambapp_find_ahb(abus, devid, 63, type, &ahbdev);
> +	if ( found == 1 ) {
> +		return 64;
> +	} else {
> +		return 63 - ahbdev.dec_index;
>  	}

Seems I have seen semi-identical code above. Factor out as function?

> +/* The define CONFIG_SYS_GRLIB_SINGLE_BUS may be defined on GRLIB systems
> + * where only one AHB Bus is available - no bridges are present. This option
> + * is available only to reduce the footprint.
> + *
> + * Defining this on a multi-bus GRLIB system may also work depending on the
> + * design.
> + */

Incorrect multiline comment style.

> +/* Get Parent bus frequency. Note that since we go from a "child" bus
> + * to a parent bus, the frequency factor direction is inverted.
> + */

Incorrect multiline comment style. Please fix globally.

> +		if ( dir ) {
> +			freq = freq * ffact;
> +		} else {
> +			freq = freq / ffact;
> +		}

No braces for oneliners. Please fix globally.

> +unsigned int gaisler_ahb2ahb_v2_freq(ambapp_ahbdev *ahb, unsigned int freq)
>  {
> -	/* find first device of this */
> -	return ambapp_ahb_scan(vendor, driver, dev, index, 1, AHB_SCAN_SLAVE);
> +	printf("gaisler_ahb2ahb_v2_freq: AHB2AHB ver 2 not supported\n");
> +	while(1) ;

Consider calling hang() instead ? [globally]

...
> +		/* Get parent bus */
> +		parent = (ioarea & 0x7);
> +		if ( parent == 0 ) {
> +			printf("ambapp_bus_freq: parent=0 indicates no parent! Stopping.\n");

Line too long. Please fix globally.

...
> +init_ahb_bridges:
> +	mov	%g0, %i0
> +	mov	%g0, %i1
> +	mov	%g0, %i2
> +	mov	%g0, %i3
> +	mov	%g0, %i4
> +	retl
> +	 mov	%g0, %i5

Incorrect indentation. Please fix globally.

...
>  void cpu_init_f(void)
>  {
> -	/* these varaiable must not be initialized */
> -	ambapp_dev_irqmp *irqmp;
> -	ambapp_apbdev apbdev;
> -	register unsigned int apbmst;
> -
> -	/* find AMBA APB Master */
> -	apbmst = (unsigned int)
> -	    ambapp_ahb_next_nomem(VENDOR_GAISLER, GAISLER_APBMST, 1, 0);
> -	if (!apbmst) {
> -		/*
> -		 * no AHB/APB bridge, something is wrong
> -		 * ==> jump to start (or hang)
> -		 */
> -		while (1) ;
> -	}
> -	/* Init memory controller */
> -	if (init_memory_ctrl()) {
> -		while (1) ;
> -	}
> -
> -	/****************************************************
> -	 * From here we can use the main memory and the stack.
> -	 */
>  
> -	/* Find AMBA APB IRQMP Controller */
> -	if (ambapp_apb_first(VENDOR_GAISLER, GAISLER_IRQMP, &apbdev) != 1) {
> -		/* no IRQ controller, something is wrong
> -		 * ==> jump to start (or hang)
> -		 */
> -		while (1) ;
> -	}
> -	irqmp = (ambapp_dev_irqmp *) apbdev.address;
> -
> -	/* initialize the IRQMP */
> -	irqmp->ilevel = 0xf;	/* all IRQ off */
> -	irqmp->iforce = 0;
> -	irqmp->ipend = 0;
> -	irqmp->iclear = 0xfffe;	/* clear all old pending interrupts */
> -	irqmp->cpu_mask[0] = 0;	/* mask all IRQs on CPU 0 */
> -	irqmp->cpu_force[0] = 0;	/* no force IRQ on CPU 0 */
> -
> -	/* cache */
>  }

Seems this becomes an empty function? Drop it, then!

> +/* Routine called from start.S,
> + *
> + * Run from FLASH/PROM:
> + *  - memory controller has already been setup up, stack can be used
> + *  - global variables available for read/writing
> + *  - constants avaiable
> + */
>  void cpu_init_f2(void)
>  {
> -
> +	/* Initialize the AMBA Plug & Play bus structure, the bus
> +	 * structure represents the AMBA bus that the CPU is located at.
> +	 */
> +	ambapp_bus_init(CONFIG_AMBAPP_IOAREA, CONFIG_SYS_CLK_FREQ, &ambapp_plb);
>  }

Or rename this into cpu_init_f() ?

> +/** Vendor GAISLER devices */
> +static ambapp_device_name GAISLER_devices[] =
> +{
> +  {GAISLER_LEON2DSU, "LEON2DSU", "Leon2 Debug Support Unit"},
> +  {GAISLER_LEON3, "LEON3", "Leon3 SPARC V8 Processor"},
> +  {GAISLER_LEON3DSU, "LEON3DSU", "Leon3 Debug Support Unit"},
> +  {GAISLER_ETHAHB, "ETHAHB", "OC ethernet AHB interface"},
...

Please use TABs for indentation - fix globally, please.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I haven't lost my mind -- it's backed up on tape somewhere.