Re: FW: Miscellaneous innovator patches

[Jonathan Larmour <jifl at eCosCentric dot com>]

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