Dear Daniel Hellstrom,
In message <1274194143-8994-3-git-send-email-dan...@gaisler.com> you wrote:
> Signed-off-by: Daniel Hellstrom <dan...@gaisler.com>
...
> - * (C) Copyright 2007
> - * Daniel Hellstrom, Gaisler Research, dan...@gaisler.com
> + * (C) Copyright 2010
> + * Daniel Hellstrom, Aeroflex Gaisler, dan...@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: w...@denx.de
I haven't lost my mind -- it's backed up on tape somewhere.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
