[PATCH] (2 commits squashed into one) Beagle BSP for review

Joel Sherrill joel.sherrill at oarcorp.com
Wed Aug 20 16:46:48 UTC 2014


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 <gedare at rtems.org
> <mailto:gedare at 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 <beng at shrike-systems.com
>     <mailto:beng at 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 <gedare at rtems.org
>     <mailto:gedare at 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
>     <beng at shrike-systems.com <mailto:beng at 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 at rtems.org <mailto:devel at rtems.org>
>     >> > http://lists.rtems.org/mailman/listinfo/devel
>     >
>     >
>     _______________________________________________
>     devel mailing list
>     devel at rtems.org <mailto:devel at rtems.org>
>     http://lists.rtems.org/mailman/listinfo/devel
>
>

-- 
Joel Sherrill, Ph.D.             Director of Research & Development
joel.sherrill at OARcorp.com        On-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35805
Support Available                (256) 722-9985

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20140820/82140217/attachment-0002.html>


More information about the devel mailing list