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

Ben Gras beng at shrike-systems.com
Mon Nov 3 12:20:43 UTC 2014 

All,

Ok, as promised, I rebased and re-tested and have found & included a
portable way of making the SD card image (included in sdcard.sh), to be
merged with RSB (i.e. some of the tools sdcard.sh relies on are missing in
mainline RSB). Some of Gedare's initial feedback is processed thanks to
Brandon Matthews. It's tested to run on the original beaglebone, beaglebone
black and qemu linaro. (The assumption is it'll run on the bbxm hardware as
well as it was before rebasing.)

The result is split into 2 patches to show what was Claas's initial work.
This makes them a bit unreadable for the final result from the patches
unfortunately.

As before, see
http://www.shrike-systems.com/beagleboard-xm-beaglebone-black-and-everything-else-rtems-on-the-beagles.html
on how to build all the tools, RTEMS executables, sdcard images, and run
the test set from linaro qemu.



On Sat, Aug 30, 2014 at 5:50 PM, Ben Gras <beng at shrike-systems.com> wrote:

> All,
>
> OK, that seems like a fruitful way to proceed to me.
>
> Then I will do some minor cleanups, rebase, do all the tests again, and
> re-submit. There's just one problem I know of that I want to fix before the
> first commit happens, and that is that the FAT fs made by mtools doesn't
> boot on the HW it seems. (It does on the emulator.) A last-minute change -
> switching to mtools instead of dosfstools to use to make the SD card image
> because the latter is so linux-only.
>
> Stay tuned.
>
>
>
>
> On Wed, Aug 20, 2014 at 4:20 PM, Gedare Bloom <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>
>> 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> 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>
>> 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
>> >> > http://lists.rtems.org/mailman/listinfo/devel
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20141103/7fe1b601/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-BeagleBoard-BSP.patch
Type: text/x-diff
Size: 138954 bytes
Desc: not available
URL: <http://lists.rtems.org/pipermail/devel/attachments/20141103/7fe1b601/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-BSP-for-several-Beagle-products.patch
Type: text/x-diff
Size: 192581 bytes
Desc: not available
URL: <http://lists.rtems.org/pipermail/devel/attachments/20141103/7fe1b601/attachment-0003.bin>

More information about the devel mailing list