<div dir="ltr"><div>Thanks for the fast & detailed review. Let me get back to it/you.<br><br></div>In the meantime, any other feedback? From anyone I mean.<br><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Thu, Jul 24, 2014 at 4:45 PM, Gedare Bloom <span dir="ltr"><<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Ben,<br>
Great work. I have a few comments. I skipped the i2c.h and i2c.c<br>
files. Most of my comments are about style and a few requests to<br>
refactor some of the larger files. The refactoring can be added to<br>
your TODO if you like. Please fix the style issues if it is not a<br>
burden.<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/README<br>
+$ ../claas-rtems/configure --target=arm-rtems4.11<br>
--enable-rtemsbsp="beaglebonewhite beagleboardxm"<br>
Replace claas-rtems with rtems. If RSB support is available, make a<br>
note about it.<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/TODO<br>
[...]<br>
open:<br>
+ . how to handle the interrupt?<br>
<br>
What does this mean?<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/clock.c<br>
Why is the entire file ifdef'd on ARM_MULTILIB_ARCH_V4?<br>
<br>
It might be sensible to put the struct definitions in a .h file if<br>
these omap registers might be re-usable.<br>
<br>
+static struct omap_timer_registers regs_v2 = {<br>
This might be better to put behind an #if IS_AM335X since it is not<br>
used otherwise?<br>
<br>
+<br>
+<br>
+<br>
Avoid more than one blank line in a row.<br>
<br>
+static int done = 0;<br>
It would be nice if you got rid of this, but otherwise give it a more<br>
useful name like "mmio_init_done"<br>
<br>
+static void beagle_clock_handler_install(rtems_interrupt_handler isr)<br>
+  if (sc != RTEMS_SUCCESSFUL) {<br>
+    rtems_fatal_error_occurred(0xdeadbeef);<br>
I think there is some capabilities in ARM for bsp fatal error codes<br>
now. They should be preferred to be used to help debug these fatal<br>
conditions.<br>
<br>
+static inline uint32_t beagle_clock_nanoseconds_since_last_tick(void)<br>
+  return (read_frc() - (uint64_t) last_tick_nanoseconds) * 1000000000<br>
/ FRCLOCK_HZ;<br>
This line is > 80 characters, please break it or shrink it.<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/console/console-config.c<br>
+#define CONSOLE_UART_LSR (*(volatile unsigned int<br>
*)(BSP_CONSOLE_UART_BASE+0x14))<br>
Line > 80 characters, even with the spacing modified.<br>
<br>
+static void beagle_console_init(void)<br>
<br>
+    while ((CONSOLE_SYSS & 1) == 0)<br>
+      ;<br>
Is this a fatal loop or is it waiting for hardware to clear something?<br>
<br>
+    if ((CONSOLE_LSR & (CONSOLE_LSR_THRE | CONSOLE_LSR_TEMT)) ==<br>
CONSOLE_LSR_THRE) {<br>
Again > 80 characters. Is the test logically equivalent to: if (<br>
(CONSOLE_LSR & CONSOLE_LSR_THRE) == CONSOLE_LSR_THRE)<br>
<br>
+    while ((CONSOLE_LSR & CONSOLE_LSR_TEMT) == 0)<br>
+      ;<br>
Is this a fatal loop or is it waiting for hardware?<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/include/bsp.h<br>
This bsp.h is really long. Probably it should be refactored into other<br>
headers, including non-public ones.<br>
<br>
+static inline void dsb(void)<br>
+{<br>
+        asm volatile("dsb" : : : "memory");<br>
Fix the indentation.<br>
<br>
+static inline void flush_data_cache(void)<br>
Perhaps this should be using _CPU_cache_flush_entire_data()? Perhaps<br>
there is a difference in that the cache manager code flushes and<br>
"cleans" the cache...<br>
<br>
+<br>
+<br>
+<br>
+<br>
Excess newlines. Done a few places in this file.<br>
<br>
The comments following the defines for various AM33X_INT_ values go<br>
off the end of the 80 column character width. Same for some other<br>
comments following defines for OMAP3_TIMER, AM33X_DMTIMER1, and<br>
AM335X_TIMER_. And further below for the CM_ WKUP and CM_PER_TIMER7<br>
defines, and CLKSEL_TIMER1MS_CLK_SEL_SEL5.<br>
<br>
+#define OMAP3_TCLR_AR       (1 << 1)  /* Autoreload or one-shot mode */<br>
+#define OMAP3_TCLR_PRE      (1 << 5)  /* Prescaler on */<br>
+#define OMAP3_TCLR_PTV      2<br>
This PTV is odd compared to the other defines here. Is it 2 == (1<<1),<br>
or is there a typo here?<br>
<br>
Tabs are used in the OMAP3_CM_ defines, it should be space characters.<br>
Also tabs are used in the read/write actlr, ttbcr, dacr, rrbr0<br>
functions and the refresh_tlb function.<br>
<br>
+/* i2c stuff */<br>
+typedef struct {<br>
...<br>
+} beagle_i2c;<br>
Shouldn't this go in beagle/include/i2c.h?<br>
<br>
All of this mmu handling code should be refactored. Where possible, it<br>
should use the existing code in arm-cp15.h<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/include/i2c.h<br>
This header defines static, non-inline functions. This doesn't make sense.<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/irq.c<br>
+static int irqs_enabled[BSP_INTERRUPT_VECTOR_MAX+1];<br>
This is an array of 512 bytes. You could use a bit vector comprising 4<br>
unsigned ints for the same purpose.<br>
<br>
+volatile static int level = 0;<br>
Unused variable?<br>
<br>
+static uint32_t get_mir_reg(int vector, uint32_t *mask)<br>
+  if(vector <   0) while(1) ;<br>
Make this a fatal error?<br>
<br>
+  if(vector <  32) return OMAP3_INTCPS_MIR0;<br>
+  if(vector <  32) return OMAP3_INTCPS_MIR0;<br>
duplicate code.<br>
<br>
+  while(1) ;<br>
Make this a fatal error?<br>
<br>
+rtems_status_code bsp_interrupt_facility_initialize(void)<br>
+  mmio_write(omap_intr.base + OMAP3_INTCPS_SYSCONFIG,<br>
OMAP3_SYSCONFIG_AUTOIDLE);<br>
Line length > 80.<br>
<br>
+++ b/c/src/lib/libbsp/arm/beagle/startup/bspstartmmu.c<br>
<br>
+//static uint32_t pagetable[ARM_SECTIONS] __attribute__((aligned (1024*16)));<br>
commented-out.. delete it?<br>
<br>
+BSP_START_TEXT_SECTION void beagle_setup_mmu_and_cache(void)<br>
__attribute__ ((weak));<br>
More than 80 characters.<br>
<br>
diff --git a/c/src/lib/libbsp/bfin/acinclude.m4<br>
b/c/src/lib/libbsp/bfin/acinclude.m4<br>
index ab6082e..828fd89 100644<br>
--- a/c/src/lib/libbsp/bfin/acinclude.m4<br>
+++ b/c/src/lib/libbsp/bfin/acinclude.m4<br>
diff --git a/c/src/lib/libbsp/powerpc/acinclude.m4<br>
b/c/src/lib/libbsp/powerpc/acinclude.m4<br>
index 6442399..e46fa2b 100644<br>
--- a/c/src/lib/libbsp/powerpc/acinclude.m4<br>
+++ b/c/src/lib/libbsp/powerpc/acinclude.m4<br>
Don't include these changes. Check your tool versions, and if the<br>
correct version of tools does this, provide a separate patch for<br>
generated files.<br>
<br>
-Gedare<br>
<div><div class="h5"><br>
On Wed, Jul 23, 2014 at 9:00 PM, Ben Gras <<a href="mailto:beng@shrike-systems.com">beng@shrike-systems.com</a>> wrote:<br>
> All,<br>
><br>
> Full details on how to reproduce all the work from source repositories to<br>
> scripts & utilities to write a complete sd card booting RTEMS and test the<br>
> whole thing:<br>
><br>
> <a href="http://www.shrike-systems.com/beagleboard-xm-beaglebone-black-and-everything-else-rtems-on-the-beagles.html" target="_blank">http://www.shrike-systems.com/beagleboard-xm-beaglebone-black-and-everything-else-rtems-on-the-beagles.html</a><br>

><br>
> I am submitting the attached patch for review for merging. If accepted for<br>
> merging, please use the top two commits on<br>
><br>
> <a href="https://github.com/bengras/rtems/tree/beaglebone-wip" target="_blank">https://github.com/bengras/rtems/tree/beaglebone-wip</a><br>
><br>
> which have the same net effect but preserve Claas' work because of the<br>
> earlier commit. The squashed version attached is for more convenient review.<br>
><br>
> I was ironing out more wrinkles but given recent interest it seems smarter<br>
> to merge sooner and keep polishing from mainline. Nevertheless I have put a<br>
> lot of work into getting it into good shape already.<br>
><br>
> I have rebased everything on the very latest master and verified<br>
><br>
> That building all the tools and utilities from scratch work, using the RTEMS<br>
> Source Builder repository (Ubuntu + FreeBSD).<br>
> That building the beaglebone and bbxm BSPs and linking them with all the<br>
> testsuite programs works (Ubuntu + FreeBSD).<br>
> That the beaglexm-emulating linaro qemu executes all of those tests<br>
> properly, invoked using a single command line with the scripts in the RTEMS<br>
> tools repository, even though not all pass currently (Ubuntu + FreeBSD).<br>
> That loading & running over JTAG works, both interactively with gdb and in a<br>
> batch using gdb and the test runner.<br>
> That running RTEMS executables using u-boot on the beaglebones from sd card<br>
> work; both with and without MMU enabled at RTEMS start time.<br>
> That Claas' earlier commit builds.<br>
><br>
> Thanks so far to Chris and Brandon for help, support, instructions and<br>
> advice in various forms :)<br>
><br>
> Test results on qemu:<br>
> Passed:   497 Failed:     3 Timeouts:   1 Invalid:    0<br>
><br>
> The test results on bbxm over jtag (older):<br>
> Passed:   475 Failed:     7 Timeouts:  10 Invalid:    0<br>
><br>
> I want to iron out more wrinkles and build support (ethernet) but giving the<br>
> bsp more exposure and having it in mainline so i don't have to keep rebasing<br>
> & testing would be nice at this point.<br>
><br>
><br>
><br>
><br>
</div></div>> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</blockquote></div><br></div>