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: New port for ARM Industrial Module AIM 711 - Checked by AntiVir DEM


Hi Andrew,

thanks for looking.

On Dienstag, 6. April 2004 15:02, Andrew Lunn wrote:
> On Wed, Mar 31, 2004 at 03:03:13PM +0200, Roland Ca?ebohm wrote:
> > Hello,
> >
> > does anybody had time to look at the code or tested the
> > run-time code?
> > Or could somebody tell me how could I help to get the
> > port integrated in the public cvs tree?
>
> Hi Roland
>
> I've started to look at this now. I will start with the HAL
>
> hal_arm_aim711.cdl:
>
> A few places you are missing the e from module.

Oh yes, I always lost it. :-(

>
> hal_platform_setup.h:
>
> Remove the commented out instruction in CYGHWR_LED_MACRO
> There is some dead code inside #if 0 which should be removed

It is for having a delay between POST codes to see where it stops
if there is a failure.
I think I make a cdl option for it.

>
> // FIXME: Is that good?
> #if CYGNUM_HAL_COMMON_INTERRUPTS_STACK_SIZE==4096
> // Override default to a more sensible value
> #undef  CYGNUM_HAL_COMMON_INTERRUPTS_STACK_SIZE
> #define CYGNUM_HAL_COMMON_INTERRUPTS_STACK_SIZE 2048
> #endif
>
> Im not so sure about this. Do other HAL do this? It seems better to do
> this in CDL.

It is copied from the snds hal, but I think I remove it, because it
does not make sense for me.

>
> This is the only HAL with a hal_aux.h file. Is this file named
> correctly? Where do other HAL put auxiliary information? Maybe in
> plf_io.h?

Yes I think you are right, other platforms have such things in
plf_io.h, so I will do that too.

>
> aim711_misc.c
>
> More code inside #if 0 to be deleted.
>
> I don't particularly like the nameing of _period. The _ suggests is a
> global variable you want to hide when in fact its a static. I also
> don't get what _period is being used for. I would rename it something
> more understandable, and without the _.

All that code is a copy of the snds and e7t hal which very semilar to
the aim711. My intention was to make one variant hal for the s3c4510
controller, which are used from the different platforms.
But till now I haven't had the time to do this, so I have just made
a copy of the snds hal and changed the things which are needed for
the aim711.

In the future the aim should support a jumper which could stop
a RedBoot scrip while start up. Thats why I have included
the cyg_plf_redboot_startup() function without content.
I will remove it till we have the whole functionality.

>
>
> In hal_IRQ_handler it looks funny dividing a status by 4. A shift
> operator would be more readable.
>

This is also from the copy of the snds hal. Maybe the best would be to
change all this, if snds, e7t and aim711 are come together to one 
variant hal with multiple platforms.

> You include <string.h> but do not use anything from it as far as i can
> see. You should not have this, it means you HAL is dependant on libc.

I needed memcpy(). Should I make a small loop to copy instead?

>
> redboot_ROMRAM.ecm:
>
> CYGDAT_REDBOOT_CUSTOM_VERSION should probably not be there. Same for
> the IP address and BOOTP configuration. It seems this is your redboot
> configuration, not a generic configuration a user would start from.

Yes, you are right, it is the version we ship with the AIM 711.
I will change this.

>
> I've not tried to actually compile it yet.
>
>        Andrew

I will change these things and what John suggested
as soon as possible and send it again to the list.

Best regards,
Roland
-- 

___________________________________________________

VS Vision Systems GmbH, Industrial Image Processing
Dipl.-Ing. Roland Caßebohm
Aspelohe 27A, D-22848 Norderstedt, Germany
http://www.visionsystems.de
___________________________________________________


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