[PATCH] (2 commits squashed into one) Beagle BSP for review
Gedare Bloom
gedare at rtems.org
Mon Nov 3 14:01:32 UTC 2014
Hi,
I don't have time to review, but am OK in principle with committing
this code as it is tested, with the caveat that my previous comments
be addressed post-merge.
If you have a diff / commits on top of what you sent before, I'd be
glad to give those a quick look.
Thanks for your contribution!
Gedare
On Mon, Nov 3, 2014 at 7:20 AM, Ben Gras <beng at shrike-systems.com> wrote:
> 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
>>> >
>>> >
>>
>>
>
More information about the devel
mailing list