This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: FW: Miscellaneous innovator patches


Doyle, Patrick wrote:
I'll go back in and scrub things up.  With this patch (or the scrubbed
version of it), I was able to run all* of the tests built with the default
template.  Since the time that I submitted it (and once I started working on
the USB support), I have discovered a number of bugs in the interrupt
handling -- it worked well enough to support the timer, but not well enough
for other devices.  Oops.  Anyway, I'll have patches for that once this one
gets committed and once I get the signoff on the copyright assignment form
here at DTC (my new company)

Good!


Speaking of which -- I think I'll do the cleanups to the last Delphi (my old
company) patch at home on my own time, so you don't need to wait for DTC to
sign paperwork, but it means that it may take a little while before I get to
it :-(

Just FYI, the way the assignment is written, you only need one assignment, and then that can cover all future contributions that you wish to make. So once the assignment for the first patch is done, you can work on the last Delphi patch wherever!


(*) IIRC, the floating point test did not run.  Probably I didn't have the
patience to allow it to run to completion -- it just seemed to be taking a
long time, so I skipped that one.  Since the test seemed to be related to
testing the hardware FPU, and since the OMAP doesn't have a hardware FPU, it
seemed just as plausable to me that a) it would take an inordinate amount of
time to run; as b) there is a bug in the software emulation of the FPU (on
the OMAP).  I didn't worry too much about it.

GCC puts in the software FP support, so it can be slow, but it may also indicate a bug in the GCC FP emulation for ARM9 :-|. So it's worth running them at least once (I certainly can't without the hardware :-)).


Thx,

Jifl

-----Original Message-----
From: Jonathan Larmour [mailto:jifl@eCosCentric.com] Sent: Monday, June 16, 2003 12:13 AM
To: Doyle, Patrick
Cc: 'ecos-patches@sources.redhat.com'
Subject: Re: FW: Miscellaneous innovator patches



Doyle, Patrick wrote:


Would now be a good time to ask about the status of these

patches again?


Yep.


I
am waiting for these to be committed, and for management

here to sign off on


the copyright assignment form, to submit new OMAP patches.

I have fixed


some interrupt related stuff and have started reorganizing

the tree to pull


all of the OMAP specific stuff into a single package, which

targets such as


the Innovator and Minno can use.

Sounds good.


With this patch, is the eCos support complete for this port now?


- //#define PLATFORM_SETUP_FROM_CCS_GEL_SCRIPT
-#ifdef PLATFORM_SETUP_FROM_CCS_GEL_SCRIPT
- // This is the version of _platform_setup adapted

from the contents


- // of the GEL script shipped with Code Composer Studio

I missed this before. I'm glad it's going though as we can't have stuff that we don't own unless we have permission to use it (unless we already discussed this and it does come with permission :)).



- // This is all stolen from the ipaq setup

iPAQ for eCos, right?



- // The rest of this is stolen from "init.c" in the

sloader program.


Ditto as above... if this is just being removed anyway, that's okay. If stuff remains and it isn't an implicit requirement with the hardware, we need to know about it.


+// -- stolen from proc-arm925.S (in Linux source)
+ mov r0, #0
+// There should be a CDL option to decide wheter we want

the streaming


+// option turned on or not

(CONFIG_CPU_ARM925_NON_STREAMING_ON). Since


+// Linux seems to work with the streaming mode disabled,

we will disable


+// it here.
+ orr r0,r0,#0x80
+#if defined(CONFIG_CPU_ARM925_TRANSPARENT_ON)
+// We need to add CDL to decide whether or not to turn transparent
+// mode on. For now, since it is disabled in Linux, we

don't enable it


+// here.
+ orr r0,r0,#0x2
+#endif
+ mcr p15, 0, r0, c15, c1, 0 @ write TI

config register


+// -- end of stuff stolen from proc-arm925.S (in Linux source)

I don't think you should say "stolen". We have to be careful about copyright. If you mean "inspired by" then say so because that's okay :).


You can't copy stuff directly, that's a no-no. If you copied ideas about the hardware setup that are sorta obvious (as in, that's the only real way to do it because the hardware has to be set up that way) then that's okay. We don't want to fall foul of copyright issues. But note: never copy anything literally.


void
hal_interrupt_mask(int vector)
{
-#ifdef LATER
+    volatile cyg_uint32 *mir;
+    cyg_uint32  old_mir;
+
    CYG_ASSERT(vector <= CYGNUM_HAL_ISR_MAX &&
               vector >= CYGNUM_HAL_ISR_MIN , "Invalid vector");

- HAL_WRITE_UINT32(INNOVATOR_INT_MASK_CLEAR, 1<<vector);
-#endif
+ // Handle a specific startup case where we are asked

to mask interrupt


+ // vector number 0. Since 'CYGNUM_HAL_ISR_MIN' is 2,

if asserts


+ // are not enabled, we will end up masking an

interrupt vector that,


+    // perhaps, we didn't want to.
+    if (vector == 0) {
+      return;
+    }

I'm not sure what the intention is here. Why is anything masking 0? But then why is it then appropriate to hit asserts in a development build? Ditto unmask.




+ // WARNING WARNING WARNING DANGER WILL ROBINSON

WARNING WARNING WARNING


+ // This will clear a pending edge triggered interrupt

that occured


+ // since the time that we received the previous edge triggered
+ // interrupt. (On the OMAP, simply determining that

we received an


+ // edge triggered interrupt clears the fact that we

received an edge


+ // triggered interrrupt -- if we receive another one,

it will be recorded


+ // in the ITR and this will clear that). This feels

like a dangerous


+ // thing to do here, but as long as all edge triggered

interrupt


+ // handlers first acknowledge the receipt of the

interrupt, and then


+ // determine the cause and/or the number of things to

do, it should be


+ // ok.

This should generally be okay because a driver should in general always loop round to determine whether there's stuff to do, rather than assume that 1 interrupt => exactly 1 event to process.



+ // Later, rinse, repeat for interrupt handler 1 if

this was an interrupt


Lather :-).


+ * Finally, the instruction stream was extracted into the

following array:


+ */
+static char idle_loop[] = {

const?



+ /* Copy the idle loop insruction stream to address

0x10000 in the DSP */


+ memcpy((void *)MPUI_PORT_RAM + 0x10000, idle_loop,

sizeof(idle_loop));


If it's important this is done byte at a time and in the right order, then be careful of using memcpy.


+2003-04-15 Patrick Doyle <wpd@dtccom.com>
+
+ * include/hal_cache.h: Added
+ 'CYGHWR_HAL_ARM_ARM9_ALT_CLEAN_DCACHE' (stolen from the

Linux code


+	for the innovator) because the 'CYGHWR_HAL_ARM_ARM9_CLEAN_CACHE'
+	feature did not work correctly on the innovator.

Nope, not the s word please! But then, I'm afraid what you've done is fairly obviously literally copied. Unfortunately that can't be used without it being fully GPL'd. Can I instead you suggest you look at the linux kernel code for _inspiration_ (if you see the distinction I mean) but then write the code from what you've now understood. Sorry if this seems picky, but we have to keep our noses clean.


The rest of the patch is absolutely fine in as much as I can tell! If you can fix/answer the above then I should have no problems checking it in.

BTW, It would be good if you could write something for the RedBoot documentation, just so people have an idea of how to get stuff onto the board. The file to look at is ecos/packages/redboot/current/doc/redboot_installing.sgml, and you can just copy an appropriate other target as a starting point if you like. If you have access to jade that would be a great help. If not, then I can probably munge around the tags and fix it up if you can make a stab at it. Just send this as a separate patch.

Jifl
--
eCosCentric http://www.eCosCentric.com/ The eCos and RedBoot experts
--[ "You can complain because roses have thorns, or you ]--
--[ can rejoice because thorns have roses." -Lincoln ]-- Opinions==mine




--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]