Hi Ben/Joel,
On Wed, Aug 20, 2014 at 10:16 PM, Joel Sherrill <joel.sherr...@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 <ged...@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 <b...@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 <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
>>
>
>
> --
> Joel Sherrill, Ph.D. Director of Research &
> developmentjoel.sherr...@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@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
