Hi Ben, Great work. I have a few comments. I skipped the i2c.h and i2c.c files. Most of my comments are about style and a few requests to refactor some of the larger files. The refactoring can be added to your TODO if you like. Please fix the style issues if it is not a burden.
+++ b/c/src/lib/libbsp/arm/beagle/README +$ ../claas-rtems/configure --target=arm-rtems4.11 --enable-rtemsbsp="beaglebonewhite beagleboardxm" Replace claas-rtems with rtems. If RSB support is available, make a note about it. +++ b/c/src/lib/libbsp/arm/beagle/TODO [...] open: + . how to handle the interrupt? What does this mean? +++ b/c/src/lib/libbsp/arm/beagle/clock.c Why is the entire file ifdef'd on ARM_MULTILIB_ARCH_V4? It might be sensible to put the struct definitions in a .h file if these omap registers might be re-usable. +static struct omap_timer_registers regs_v2 = { This might be better to put behind an #if IS_AM335X since it is not used otherwise? + + + Avoid more than one blank line in a row. +static int done = 0; It would be nice if you got rid of this, but otherwise give it a more useful name like "mmio_init_done" +static void beagle_clock_handler_install(rtems_interrupt_handler isr) + if (sc != RTEMS_SUCCESSFUL) { + rtems_fatal_error_occurred(0xdeadbeef); I think there is some capabilities in ARM for bsp fatal error codes now. They should be preferred to be used to help debug these fatal conditions. +static inline uint32_t beagle_clock_nanoseconds_since_last_tick(void) + return (read_frc() - (uint64_t) last_tick_nanoseconds) * 1000000000 / FRCLOCK_HZ; This line is > 80 characters, please break it or shrink it. +++ b/c/src/lib/libbsp/arm/beagle/console/console-config.c +#define CONSOLE_UART_LSR (*(volatile unsigned int *)(BSP_CONSOLE_UART_BASE+0x14)) Line > 80 characters, even with the spacing modified. +static void beagle_console_init(void) + while ((CONSOLE_SYSS & 1) == 0) + ; Is this a fatal loop or is it waiting for hardware to clear something? + if ((CONSOLE_LSR & (CONSOLE_LSR_THRE | CONSOLE_LSR_TEMT)) == CONSOLE_LSR_THRE) { Again > 80 characters. Is the test logically equivalent to: if ( (CONSOLE_LSR & CONSOLE_LSR_THRE) == CONSOLE_LSR_THRE) + while ((CONSOLE_LSR & CONSOLE_LSR_TEMT) == 0) + ; Is this a fatal loop or is it waiting for hardware? +++ b/c/src/lib/libbsp/arm/beagle/include/bsp.h This bsp.h is really long. Probably it should be refactored into other headers, including non-public ones. +static inline void dsb(void) +{ + asm volatile("dsb" : : : "memory"); Fix the indentation. +static inline void flush_data_cache(void) Perhaps this should be using _CPU_cache_flush_entire_data()? Perhaps there is a difference in that the cache manager code flushes and "cleans" the cache... + + + + Excess newlines. Done a few places in this file. The comments following the defines for various AM33X_INT_ values go off the end of the 80 column character width. Same for some other comments following defines for OMAP3_TIMER, AM33X_DMTIMER1, and AM335X_TIMER_. And further below for the CM_ WKUP and CM_PER_TIMER7 defines, and CLKSEL_TIMER1MS_CLK_SEL_SEL5. +#define OMAP3_TCLR_AR (1 << 1) /* Autoreload or one-shot mode */ +#define OMAP3_TCLR_PRE (1 << 5) /* Prescaler on */ +#define OMAP3_TCLR_PTV 2 This PTV is odd compared to the other defines here. Is it 2 == (1<<1), or is there a typo here? Tabs are used in the OMAP3_CM_ defines, it should be space characters. Also tabs are used in the read/write actlr, ttbcr, dacr, rrbr0 functions and the refresh_tlb function. +/* i2c stuff */ +typedef struct { ... +} beagle_i2c; Shouldn't this go in beagle/include/i2c.h? All of this mmu handling code should be refactored. Where possible, it should use the existing code in arm-cp15.h +++ b/c/src/lib/libbsp/arm/beagle/include/i2c.h This header defines static, non-inline functions. This doesn't make sense. +++ b/c/src/lib/libbsp/arm/beagle/irq.c +static int irqs_enabled[BSP_INTERRUPT_VECTOR_MAX+1]; This is an array of 512 bytes. You could use a bit vector comprising 4 unsigned ints for the same purpose. +volatile static int level = 0; Unused variable? +static uint32_t get_mir_reg(int vector, uint32_t *mask) + if(vector < 0) while(1) ; Make this a fatal error? + if(vector < 32) return OMAP3_INTCPS_MIR0; + if(vector < 32) return OMAP3_INTCPS_MIR0; duplicate code. + while(1) ; Make this a fatal error? +rtems_status_code bsp_interrupt_facility_initialize(void) + mmio_write(omap_intr.base + OMAP3_INTCPS_SYSCONFIG, OMAP3_SYSCONFIG_AUTOIDLE); Line length > 80. +++ b/c/src/lib/libbsp/arm/beagle/startup/bspstartmmu.c +//static uint32_t pagetable[ARM_SECTIONS] __attribute__((aligned (1024*16))); commented-out.. delete it? +BSP_START_TEXT_SECTION void beagle_setup_mmu_and_cache(void) __attribute__ ((weak)); More than 80 characters. diff --git a/c/src/lib/libbsp/bfin/acinclude.m4 b/c/src/lib/libbsp/bfin/acinclude.m4 index ab6082e..828fd89 100644 --- a/c/src/lib/libbsp/bfin/acinclude.m4 +++ b/c/src/lib/libbsp/bfin/acinclude.m4 diff --git a/c/src/lib/libbsp/powerpc/acinclude.m4 b/c/src/lib/libbsp/powerpc/acinclude.m4 index 6442399..e46fa2b 100644 --- a/c/src/lib/libbsp/powerpc/acinclude.m4 +++ b/c/src/lib/libbsp/powerpc/acinclude.m4 Don't include these changes. Check your tool versions, and if the correct version of tools does this, provide a separate patch for generated files. -Gedare On Wed, Jul 23, 2014 at 9:00 PM, Ben Gras <b...@shrike-systems.com> wrote: > All, > > Full details on how to reproduce all the work from source repositories to > scripts & utilities to write a complete sd card booting RTEMS and test the > whole thing: > > http://www.shrike-systems.com/beagleboard-xm-beaglebone-black-and-everything-else-rtems-on-the-beagles.html > > I am submitting the attached patch for review for merging. If accepted for > merging, please use the top two commits on > > https://github.com/bengras/rtems/tree/beaglebone-wip > > which have the same net effect but preserve Claas' work because of the > earlier commit. The squashed version attached is for more convenient review. > > I was ironing out more wrinkles but given recent interest it seems smarter > to merge sooner and keep polishing from mainline. Nevertheless I have put a > lot of work into getting it into good shape already. > > I have rebased everything on the very latest master and verified > > That building all the tools and utilities from scratch work, using the RTEMS > Source Builder repository (Ubuntu + FreeBSD). > That building the beaglebone and bbxm BSPs and linking them with all the > testsuite programs works (Ubuntu + FreeBSD). > That the beaglexm-emulating linaro qemu executes all of those tests > properly, invoked using a single command line with the scripts in the RTEMS > tools repository, even though not all pass currently (Ubuntu + FreeBSD). > That loading & running over JTAG works, both interactively with gdb and in a > batch using gdb and the test runner. > That running RTEMS executables using u-boot on the beaglebones from sd card > work; both with and without MMU enabled at RTEMS start time. > That Claas' earlier commit builds. > > Thanks so far to Chris and Brandon for help, support, instructions and > advice in various forms :) > > Test results on qemu: > Passed: 497 Failed: 3 Timeouts: 1 Invalid: 0 > > The test results on bbxm over jtag (older): > Passed: 475 Failed: 7 Timeouts: 10 Invalid: 0 > > I want to iron out more wrinkles and build support (ethernet) but giving the > bsp more exposure and having it in mainline so i don't have to keep rebasing > & testing would be nice at this point. > > > > > _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel