[Bug 1801] smp support for the sparc

[bugzilla-daemon at rtems.org]

https://www.rtems.org/bugzilla/show_bug.cgi?id=1801

Gedare <giddyup44 at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |giddyup44 at yahoo.com

--- Comment #2 from Gedare <giddyup44 at yahoo.com> 2011-05-19 10:20:05 CDT ---
I personally prefer to see includes, macros, and global variables near the top
of a file or in a header file. Same goes for function prototypes. Do we have a
coding style handbook anywhere? I know it is hard to converge on a given style.
Anyway, a number of "violations" (to my eye) appear in this patch:
smp_leon3.c:
+int rtems_bsp_smp_cpus = 0;
A random, uncommented global variable? Should this be declared static in this
file, or is it exported by a header file somewhere else in RTEMS?

+#include <rtems/bspsmp.h>
Redundantly included part-way in file.

+#define LEON3_IRQMPSTATUS_CPUNR     28
+#define LEON3_IRQMPSTATUS_BROADCAST 27
+
+extern void *bsp_ap_stack;
+extern void *bsp_ap_entry;
+
+void bsp_smp_delay( int );
+
All of these should almost certainly be in a header file somewhere that would
be included in this file.


There are also some stray printks that should be eliminated or wrapped in DEBUG
ifdefs:
smp_leon3.c:
+  printk( "Found %d CPUs\n", rtems_bsp_smp_cpus );

+    printk(
+      "%d CPUs IS MORE THAN CONFIGURED -- ONLY USING %d\n",
+      rtems_bsp_smp_cpus,
+      rtems_configuration_smp_maximum_processors
+    );

+    printk( "Waking CPU %d\n", cpu );

+    printk(
+      "CPU %d is %s\n",
+      cpu,
+      ((_Per_CPU_Information[cpu].state == RTEMS_BSP_SMP_CPU_INITIALIZED) ?
+         "online" : "offline")
+    );


ap_ipi_isr():
Rename: bsp_ap_ipi_isr()?

+  /*
+   *  Broadcast isn't working just yet given the ISR code shares variables.
+   *  When it does, we should be able to use the broadcast.
+   */
+#if 1
Is this comment accurate, and Joel already commented on the #if 1.

+extern __inline__ void __delay(unsigned long loops)
Why using this function? FWIW this is more like what I expected rtems_bsp_delay
would look like.

+void bsp_smp_delay( int max )
Not sure what this function is doing.

void bsp_smp_wait_for():
+  volatile int i;
unused variable

+    bsp_smp_delay( 5000 );
Why not using rtems_bsp_delay()

start.S:
+#if defined(RTEMS_SMP) && BSP_LEON3_SMP 
I think you also want defined(BSP_LEON3_SMP)

+    mov    %sp, %fp
+    nop
Spurious nop?

+#if defined(ENABLE_SMP)
+    .common bsp_ap_stack, 4, 4
+    .common bsp_ap_entry, 4, 4
+#endif
Is this the best place to declare these? Probably there is a nicer header file
to declare these variables?

-- 
Configure bugmail: https://www.rtems.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.