On 8/20/2014 11:23 AM, Ritesh Harjani wrote: > Hi Ben, > > Great work I must admit. > > I just have few question/suggestion. I am new to the community so high > chances that I might be wrong here, but I noticed something so I > thought I should tell/ask. > > > diff --git a/c/src/lib/libbsp/arm/beagle/include/bsp.h > b/c/src/lib/libbsp/arm/beagle/include/bsp.h > new file mode 100644 > index 0000000..fc001dd1 > --- /dev/null > +++ b/c/src/lib/libbsp/arm/beagle/include/bsp.h > ... > ... > +/* Data synchronization barrier */ > +static inline void dsb(void) > +{ > + asm volatile("dsb" : : : "memory"); > +} > + > +/* Instruction synchronization barrier */ > +static inline void isb(void) > +{ > + asm volatile("isb" : : : "memory"); > +} > ... > ... > I guess these similar barrier functions are already present > in cpukit/score/cpu/arm/rtems/score/cpu.h. > So, do we need to add this again ? > Good catch. Ben they are in cpukit/score/cpu/arm/rtems/score/cpu.h which is implicitly included.
I can't there there is a CPU independent wrapper for synchronizations like this. > > +/* flush data cache */ > +static inline void flush_data_cache(void) > +{ > + asm volatile("mov r0, #0; mcr p15, #0, r0, c7, c10, #4" : : : > "memory"); > +} > > the name of this function is confusing to me. This is not a flush > operation right ? Following link says it is a data synchronization > operation. > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0344c/Babhejba.html > > Please correct me if I am wrong at any place. > > Thanks > Ritesh > > > > On Wed, Aug 20, 2014 at 7:50 PM, Gedare Bloom <ged...@rtems.org > <mailto:ged...@rtems.org>> wrote: > > Ben, As far as getting this merged, all of my comments can be done as > a follow-on commit. -Gedare > > On Thu, Jul 24, 2014 at 4:28 PM, Ben Gras <b...@shrike-systems.com > <mailto:b...@shrike-systems.com>> wrote: > > Thanks for the fast & detailed review. Let me get back to it/you. > > > > In the meantime, any other feedback? From anyone I mean. > > > > > > > > On Thu, Jul 24, 2014 at 4:45 PM, Gedare Bloom <ged...@rtems.org > <mailto:ged...@rtems.org>> wrote: > >> > >> 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 <mailto: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 <mailto:devel@rtems.org> > >> > http://lists.rtems.org/mailman/listinfo/devel > > > > > _______________________________________________ > devel mailing list > devel@rtems.org <mailto:devel@rtems.org> > http://lists.rtems.org/mailman/listinfo/devel > > -- Joel Sherrill, Ph.D. Director of Research & Development joel.sherr...@oarcorp.com On-Line Applications Research Ask me about RTEMS: a free RTOS Huntsville AL 35805 Support Available (256) 722-9985
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel