<div dir="ltr">Hi Ben/Joel, <div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 20, 2014 at 10:16 PM, Joel Sherrill <span dir="ltr"><<a href="mailto:joel.sherrill@oarcorp.com" target="_blank">joel.sherrill@oarcorp.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div class="h5">
<br>
<div>On 8/20/2014 11:23 AM, Ritesh Harjani
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi Ben,
<div><br>
</div>
<div>Great work I must admit. </div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div><br>
</div>
<div>
<div>diff --git a/c/src/lib/libbsp/arm/beagle/include/bsp.h
b/c/src/lib/libbsp/arm/beagle/include/bsp.h</div>
<div>new file mode 100644</div>
<div>index 0000000..fc001dd1</div>
<div>--- /dev/null</div>
<div>+++ b/c/src/lib/libbsp/arm/beagle/include/bsp.h</div>
</div>
<div>...</div>
<div>...</div>
<div>
<div>+/* Data synchronization barrier */</div>
<div>+static inline void dsb(void)</div>
<div>+{</div>
<div>+ asm volatile("dsb" : : : "memory");</div>
<div>+}</div>
<div>+</div>
<div>+/* Instruction synchronization barrier */</div>
<div>+static inline void isb(void)</div>
<div>+{</div>
<div>+ asm volatile("isb" : : : "memory");</div>
<div>+}</div>
</div>
<div>...</div>
<div>...</div>
<div>I guess these similar barrier functions are already present
in cpukit/score/cpu/arm/rtems/score/cpu.h. </div>
<div>So, do we need to add this again ? </div>
<div><br>
</div>
</div>
</blockquote></div></div>
Good catch. Ben they are in cpukit/score/cpu/arm/rtems/score/cpu.h
which is implicitly included.<br>
<br>
I can't there there is a CPU independent wrapper for
synchronizations like this. <br></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
<div><div class="h5"><blockquote type="cite"><div dir="ltr"><div>+/* flush data cache */<br></div><div>
<div>+static inline void flush_data_cache(void)</div>
<div>+{</div>
<div>+ asm volatile("mov r0, #0; mcr p15, #0, r0, c7,
c10, #4" : : : "memory");</div>
<div>+}</div>
</div>
<div><br>
</div>
<div>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. </div>
<div><a href="http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0344c/Babhejba.html" target="_blank">http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0344c/Babhejba.html</a></div></div>
</blockquote></div></div></div></blockquote><div>Even the above function I see in here. I am not sure if this should be used as below: </div><div><br></div><div><div>Implemented in c/src/lib/libcpu/arm/shared/include/arm-cp15.h</div>
<div>Atleast the assembly syntax is the same. </div><div><br></div><div><div>ARM_CP15_TEXT_SECTION static inline void</div><div>arm_cp15_drain_write_buffer(void)</div><div>{</div><div> ARM_SWITCH_REGISTERS;</div><div> uint32_t sbz = 0;</div>
<div><br></div><div> __asm__ volatile (</div><div> ARM_SWITCH_TO_ARM</div><div> "mcr p15, 0, %[sbz], c7, c10, 4\n"</div><div> ARM_SWITCH_BACK</div><div> : ARM_SWITCH_OUTPUT</div><div> : [sbz] "r" (sbz)</div>
<div> : "memory"</div><div> );</div><div>}</div></div></div><div><br></div><div><br></div><div><br></div><div>Thanks</div><div>Ritesh </div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div class="h5"><blockquote type="cite"><div dir="ltr"><div><br>
</div>
<div><br>
</div>
</div>
</blockquote>
<br>
<blockquote type="cite">
<div dir="ltr">
<div>Please correct me if I am wrong at any place. </div>
<div><br>
</div>
<div>Thanks</div>
<div>Ritesh </div>
<div><br>
</div>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">On Wed, Aug 20, 2014 at 7:50 PM,
Gedare Bloom <span dir="ltr"><<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Ben,
As far as getting this merged, all of my comments can be
done as<br>
a follow-on commit. -Gedare<br>
<div>
<div><br>
On Thu, Jul 24, 2014 at 4:28 PM, Ben Gras <<a href="mailto:beng@shrike-systems.com" target="_blank">beng@shrike-systems.com</a>>
wrote:<br>
> Thanks for the fast & detailed review. Let me
get back to it/you.<br>
><br>
> In the meantime, any other feedback? From anyone
I mean.<br>
><br>
><br>
><br>
> On Thu, Jul 24, 2014 at 4:45 PM, Gedare Bloom
<<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>>
wrote:<br>
>><br>
>> Hi Ben,<br>
>> Great work. I have a few comments. I skipped
the i2c.h and i2c.c<br>
>> files. Most of my comments are about style
and a few requests to<br>
>> refactor some of the larger files. The
refactoring can be added to<br>
>> your TODO if you like. Please fix the style
issues if it is not a<br>
>> burden.<br>
>><br>
>> +++ b/c/src/lib/libbsp/arm/beagle/README<br>
>> +$ ../claas-rtems/configure
--target=arm-rtems4.11<br>
>> --enable-rtemsbsp="beaglebonewhite
beagleboardxm"<br>
>> Replace claas-rtems with rtems. If RSB
support is available, make a<br>
>> note about it.<br>
>><br>
>> +++ b/c/src/lib/libbsp/arm/beagle/TODO<br>
>> [...]<br>
>> open:<br>
>> + . how to handle the interrupt?<br>
>><br>
>> What does this mean?<br>
>><br>
>> +++ b/c/src/lib/libbsp/arm/beagle/clock.c<br>
>> Why is the entire file ifdef'd on
ARM_MULTILIB_ARCH_V4?<br>
>><br>
>> It might be sensible to put the struct
definitions in a .h file if<br>
>> these omap registers might be re-usable.<br>
>><br>
>> +static struct omap_timer_registers regs_v2 =
{<br>
>> This might be better to put behind an #if
IS_AM335X since it is not<br>
>> used otherwise?<br>
>><br>
>> +<br>
>> +<br>
>> +<br>
>> Avoid more than one blank line in a row.<br>
>><br>
>> +static int done = 0;<br>
>> It would be nice if you got rid of this, but
otherwise give it a more<br>
>> useful name like "mmio_init_done"<br>
>><br>
>> +static void
beagle_clock_handler_install(rtems_interrupt_handler
isr)<br>
>> + if (sc != RTEMS_SUCCESSFUL) {<br>
>> + rtems_fatal_error_occurred(0xdeadbeef);<br>
>> I think there is some capabilities in ARM for
bsp fatal error codes<br>
>> now. They should be preferred to be used to
help debug these fatal<br>
>> conditions.<br>
>><br>
>> +static inline uint32_t
beagle_clock_nanoseconds_since_last_tick(void)<br>
>> + return (read_frc() - (uint64_t)
last_tick_nanoseconds) * 1000000000<br>
>> / FRCLOCK_HZ;<br>
>> This line is > 80 characters, please break
it or shrink it.<br>
>><br>
>> +++
b/c/src/lib/libbsp/arm/beagle/console/console-config.c<br>
>> +#define CONSOLE_UART_LSR (*(volatile
unsigned int<br>
>> *)(BSP_CONSOLE_UART_BASE+0x14))<br>
>> Line > 80 characters, even with the
spacing modified.<br>
>><br>
>> +static void beagle_console_init(void)<br>
>><br>
>> + while ((CONSOLE_SYSS & 1) == 0)<br>
>> + ;<br>
>> Is this a fatal loop or is it waiting for
hardware to clear something?<br>
>><br>
>> + if ((CONSOLE_LSR & (CONSOLE_LSR_THRE
| CONSOLE_LSR_TEMT)) ==<br>
>> CONSOLE_LSR_THRE) {<br>
>> Again > 80 characters. Is the test
logically equivalent to: if (<br>
>> (CONSOLE_LSR & CONSOLE_LSR_THRE) ==
CONSOLE_LSR_THRE)<br>
>><br>
>> + while ((CONSOLE_LSR &
CONSOLE_LSR_TEMT) == 0)<br>
>> + ;<br>
>> Is this a fatal loop or is it waiting for
hardware?<br>
>><br>
>> +++
b/c/src/lib/libbsp/arm/beagle/include/bsp.h<br>
>> This bsp.h is really long. Probably it should
be refactored into other<br>
>> headers, including non-public ones.<br>
>><br>
>> +static inline void dsb(void)<br>
>> +{<br>
>> + asm volatile("dsb" : : : "memory");<br>
>> Fix the indentation.<br>
>><br>
>> +static inline void flush_data_cache(void)<br>
>> Perhaps this should be using
_CPU_cache_flush_entire_data()? Perhaps<br>
>> there is a difference in that the cache
manager code flushes and<br>
>> "cleans" the cache...<br>
>><br>
>> +<br>
>> +<br>
>> +<br>
>> +<br>
>> Excess newlines. Done a few places in this
file.<br>
>><br>
>> The comments following the defines for
various AM33X_INT_ values go<br>
>> off the end of the 80 column character width.
Same for some other<br>
>> comments following defines for OMAP3_TIMER,
AM33X_DMTIMER1, and<br>
>> AM335X_TIMER_. And further below for the CM_
WKUP and CM_PER_TIMER7<br>
>> defines, and CLKSEL_TIMER1MS_CLK_SEL_SEL5.<br>
>><br>
>> +#define OMAP3_TCLR_AR (1 << 1)
/* Autoreload or one-shot mode */<br>
>> +#define OMAP3_TCLR_PRE (1 << 5)
/* Prescaler on */<br>
>> +#define OMAP3_TCLR_PTV 2<br>
>> This PTV is odd compared to the other defines
here. Is it 2 == (1<<1),<br>
>> or is there a typo here?<br>
>><br>
>> Tabs are used in the OMAP3_CM_ defines, it
should be space characters.<br>
>> Also tabs are used in the read/write actlr,
ttbcr, dacr, rrbr0<br>
>> functions and the refresh_tlb function.<br>
>><br>
>> +/* i2c stuff */<br>
>> +typedef struct {<br>
>> ...<br>
>> +} beagle_i2c;<br>
>> Shouldn't this go in beagle/include/i2c.h?<br>
>><br>
>> All of this mmu handling code should be
refactored. Where possible, it<br>
>> should use the existing code in arm-cp15.h<br>
>><br>
>> +++
b/c/src/lib/libbsp/arm/beagle/include/i2c.h<br>
>> This header defines static, non-inline
functions. This doesn't make sense.<br>
>><br>
>> +++ b/c/src/lib/libbsp/arm/beagle/irq.c<br>
>> +static int
irqs_enabled[BSP_INTERRUPT_VECTOR_MAX+1];<br>
>> This is an array of 512 bytes. You could use
a bit vector comprising 4<br>
>> unsigned ints for the same purpose.<br>
>><br>
>> +volatile static int level = 0;<br>
>> Unused variable?<br>
>><br>
>> +static uint32_t get_mir_reg(int vector,
uint32_t *mask)<br>
>> + if(vector < 0) while(1) ;<br>
>> Make this a fatal error?<br>
>><br>
>> + if(vector < 32) return
OMAP3_INTCPS_MIR0;<br>
>> + if(vector < 32) return
OMAP3_INTCPS_MIR0;<br>
>> duplicate code.<br>
>><br>
>> + while(1) ;<br>
>> Make this a fatal error?<br>
>><br>
>> +rtems_status_code
bsp_interrupt_facility_initialize(void)<br>
>> + mmio_write(omap_intr.base +
OMAP3_INTCPS_SYSCONFIG,<br>
>> OMAP3_SYSCONFIG_AUTOIDLE);<br>
>> Line length > 80.<br>
>><br>
>> +++
b/c/src/lib/libbsp/arm/beagle/startup/bspstartmmu.c<br>
>><br>
>> +//static uint32_t pagetable[ARM_SECTIONS]
__attribute__((aligned<br>
>> (1024*16)));<br>
>> commented-out.. delete it?<br>
>><br>
>> +BSP_START_TEXT_SECTION void
beagle_setup_mmu_and_cache(void)<br>
>> __attribute__ ((weak));<br>
>> More than 80 characters.<br>
>><br>
>> diff --git
a/c/src/lib/libbsp/bfin/acinclude.m4<br>
>> b/c/src/lib/libbsp/bfin/acinclude.m4<br>
>> index ab6082e..828fd89 100644<br>
>> --- a/c/src/lib/libbsp/bfin/acinclude.m4<br>
>> +++ b/c/src/lib/libbsp/bfin/acinclude.m4<br>
>> diff --git
a/c/src/lib/libbsp/powerpc/acinclude.m4<br>
>> b/c/src/lib/libbsp/powerpc/acinclude.m4<br>
>> index 6442399..e46fa2b 100644<br>
>> --- a/c/src/lib/libbsp/powerpc/acinclude.m4<br>
>> +++ b/c/src/lib/libbsp/powerpc/acinclude.m4<br>
>> Don't include these changes. Check your tool
versions, and if the<br>
>> correct version of tools does this, provide a
separate patch for<br>
>> generated files.<br>
>><br>
>> -Gedare<br>
>><br>
>> On Wed, Jul 23, 2014 at 9:00 PM, Ben Gras
<<a href="mailto:beng@shrike-systems.com" target="_blank">beng@shrike-systems.com</a>>
wrote:<br>
>> > All,<br>
>> ><br>
>> > Full details on how to reproduce all the
work from source repositories<br>
>> > to<br>
>> > scripts & utilities to write a
complete sd card booting RTEMS and test<br>
>> > the<br>
>> > whole thing:<br>
>> ><br>
>> ><br>
>> > <a href="http://www.shrike-systems.com/beagleboard-xm-beaglebone-black-and-everything-else-rtems-on-the-beagles.html" target="_blank">http://www.shrike-systems.com/beagleboard-xm-beaglebone-black-and-everything-else-rtems-on-the-beagles.html</a><br>
>> ><br>
>> > I am submitting the attached patch for
review for merging. If accepted<br>
>> > for<br>
>> > merging, please use the top two commits
on<br>
>> ><br>
>> > <a href="https://github.com/bengras/rtems/tree/beaglebone-wip" target="_blank">https://github.com/bengras/rtems/tree/beaglebone-wip</a><br>
>> ><br>
>> > which have the same net effect but
preserve Claas' work because of the<br>
>> > earlier commit. The squashed version
attached is for more convenient<br>
>> > review.<br>
>> ><br>
>> > I was ironing out more wrinkles but
given recent interest it seems<br>
>> > smarter<br>
>> > to merge sooner and keep polishing from
mainline. Nevertheless I have<br>
>> > put a<br>
>> > lot of work into getting it into good
shape already.<br>
>> ><br>
>> > I have rebased everything on the very
latest master and verified<br>
>> ><br>
>> > That building all the tools and
utilities from scratch work, using the<br>
>> > RTEMS<br>
>> > Source Builder repository (Ubuntu +
FreeBSD).<br>
>> > That building the beaglebone and bbxm
BSPs and linking them with all the<br>
>> > testsuite programs works (Ubuntu +
FreeBSD).<br>
>> > That the beaglexm-emulating linaro qemu
executes all of those tests<br>
>> > properly, invoked using a single command
line with the scripts in the<br>
>> > RTEMS<br>
>> > tools repository, even though not all
pass currently (Ubuntu + FreeBSD).<br>
>> > That loading & running over JTAG
works, both interactively with gdb and<br>
>> > in a<br>
>> > batch using gdb and the test runner.<br>
>> > That running RTEMS executables using
u-boot on the beaglebones from sd<br>
>> > card<br>
>> > work; both with and without MMU enabled
at RTEMS start time.<br>
>> > That Claas' earlier commit builds.<br>
>> ><br>
>> > Thanks so far to Chris and Brandon for
help, support, instructions and<br>
>> > advice in various forms :)<br>
>> ><br>
>> > Test results on qemu:<br>
>> > Passed: 497 Failed: 3 Timeouts:
1 Invalid: 0<br>
>> ><br>
>> > The test results on bbxm over jtag
(older):<br>
>> > Passed: 475 Failed: 7 Timeouts:
10 Invalid: 0<br>
>> ><br>
>> > I want to iron out more wrinkles and
build support (ethernet) but giving<br>
>> > the<br>
>> > bsp more exposure and having it in
mainline so i don't have to keep<br>
>> > rebasing<br>
>> > & testing would be nice at this
point.<br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> >
_______________________________________________<br>
>> > devel mailing list<br>
>> > <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
>> > <a href="http://lists.rtems.org/mailman/listinfo/devel" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
><br>
><br>
_______________________________________________<br>
devel mailing list<br>
<a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
<a href="http://lists.rtems.org/mailman/listinfo/devel" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div></div><span class=""><font color="#888888"><pre cols="72">--
Joel Sherrill, Ph.D. Director of Research & Development
<a href="mailto:joel.sherrill@OARcorp.com" target="_blank">joel.sherrill@OARcorp.com</a> On-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available (256) 722-9985</pre>
</font></span></div>
</blockquote></div><br></div></div></div>