[Bug 1801] smp support for the sparc

[bugzilla-daemon at rtems.org]

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

--- Comment #3 from Joel Sherrill <joel.sherrill at oarcorp.com> 2011-06-28 15:29:39 CDT ---
I had a really long and detailed reply.  Then I lost it.

I have attempted to deal with all of the issues.  The code is committed.  So
just comment on that.

If OK, then close the PR please.

(In reply to comment #2)
> 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.