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> 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> 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
> > http://lists.rtems.org/mailman/listinfo/devel
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
