[PATCH] (2 commits squashed into one) Beagle BSP for review
Brandon Matthews
rtems at optimaltour.us
Wed Aug 20 21:07:47 UTC 2014
Thanks for the update and input, all. I'll make sure it's included in our
current issue tracking.
FYI, work on this has briefly stalled, as Ben is on holiday and I'm working
on wrapping up my current employment in favor of something new. I intend to
begin working again when I have some free time next week and the week after.
Thanks,
Brandon
On Wed, Aug 20, 2014 at 9:53 AM, Ritesh Harjani <ritesh.harjani at gmail.com>
wrote:
> Hi Ben/Joel,
>
> On Wed, Aug 20, 2014 at 10:16 PM, Joel Sherrill <joel.sherrill at oarcorp.com
> > wrote:
>
>>
>> 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
>>
>> Even the above function I see in here. I am not sure if this should be
> used as below:
>
> Implemented in c/src/lib/libcpu/arm/shared/include/arm-cp15.h
> Atleast the assembly syntax is the same.
>
> ARM_CP15_TEXT_SECTION static inline void
> arm_cp15_drain_write_buffer(void)
> {
> ARM_SWITCH_REGISTERS;
> uint32_t sbz = 0;
>
> __asm__ volatile (
> ARM_SWITCH_TO_ARM
> "mcr p15, 0, %[sbz], c7, c10, 4\n"
> ARM_SWITCH_BACK
> : ARM_SWITCH_OUTPUT
> : [sbz] "r" (sbz)
> : "memory"
> );
> }
>
>
>
> Thanks
> Ritesh
>
>
>
>
>>
>>
>>
>> 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> 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
>>> >
>>> >
>>> _______________________________________________
>>> devel mailing list
>>> devel at rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel
>>>
>>
>>
>> --
>> Joel Sherrill, Ph.D. Director of Research & Developmentjoel.sherrill at OARcorp.com On-Line Applications Research
>> Ask me about RTEMS: a free RTOS Huntsville AL 35805
>> Support Available (256) 722-9985
>>
>>
>
> _______________________________________________
> 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/20140820/00704fe1/attachment-0002.html>
More information about the devel
mailing list