[PATCH] (2 commits squashed into one) Beagle BSP for review
Joel Sherrill
joel.sherrill at oarcorp.com
Mon Nov 3 21:23:25 UTC 2014
On 11/3/2014 3:06 PM, Ben Gras wrote:
> All,
>
>
> Joel merged these and I updated my blog post to reflect the mainline
> repo. Thanks Joel!
>
Thank you Ben for the nice submission!!!
Now to make sure it is reproducible from here and we have merged
all the bits into the tools, etc.
> On Mon, Nov 3, 2014 at 8:40 PM, Ben Gras <beng at shrike-systems.com
> <mailto:beng at shrike-systems.com>> wrote:
>
> All,
>
> I have new patches with some last-minute smoothings added; removed
> obsolete beagle.cfg, TODO, and separated the more generic ARM
> headers into a separate commit. The new 3 commits are attached
> (and in my RTEMS github repo).
>
> Gedare, there is also a diff w.r.t. the previous submission
> attached as requested.
>
>
>
> On Mon, Nov 3, 2014 at 3:01 PM, Gedare Bloom <gedare at rtems.org
> <mailto:gedare at rtems.org>> wrote:
>
> 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 <mailto: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 <mailto: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 <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
> >>> >
> >>> >
> >>
> >>
> >
>
>
>
--
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/20141103/fa63adf4/attachment-0002.html>
More information about the devel
mailing list