[Bug 1000280] New port: Freescale MAC7100 Variant, Freescale ESCI serial driver

[bugzilla-daemon at ecoscentric dot com]

http://bugzilla.ecoscentric.com/show_bug.cgi?id=1000280





------- Additional Comments From jifl@ecoscentric.com  2006-05-29 18:30 -------
The following was sent by Ilija by mail to bugzilla-daemon instead of replied in
the bug:

> ------- Additional Comments From andrew.lunn@ascom.ch  2006-05-27 14:15 -------
> In hal_arm_mac7100.cdl you have:
>
>    requires { CYGPKG_IO_SERIAL_DEVICES == 1 }
>
> Why? Is this just left over from debugging? Can i remove it?
>   

This is because hal_diag.c and ser_esci.c share same header file ser_esci.h
Other ports (i.e. at91) define these macros in var_io.h that limits it's usage
to particular variant.
My idea is to enable usage of ser_esci with other freescale chips that implement
it such as MPC5500 (latest Freescale PowerPC automotive family).
For that reason I have decoupled serial driver from hal. Serial port macros stay
together with driver and the statement:

    requires { CYGPKG_IO_SERIAL_DEVICES == 1 }

is in order to instruct configtool to include ser_esci.h even in case serial
port is not selected (by the user).
I guess the only harm may be longer compilation time and  a little bit more disk
space, since ser_esci.c may be compiled but it won't affect target code.

Can you suggest some other trick?

> You have the base addresses and interrupts of the serial devices in CDL. This is
> not normal. They are normally in var_io.h or plf_io.h. What is the reason you
> did it this way?
>   

I was leaded by previous reason... And maybe I was a little bit too impressed
having something like configtool so i over do (give people something useful and
they'll use it).
hal_io is not practical since all MAC7100 peripheral addresses are fixed within
family map so I placed macros in var_io.h

> Please could you add some documentation to flash_security.S. It is not clear
> what this file does from reading its contents, and seeing words like "backdoor"
> is not good for a maintainer!
>   

There isn't anything to change in this section unless user wishes to protect
Flash contents (it will lock JTAG too). Chosen configuration is default factory
setting.
Detail description of flash security is given in Freescale's ref. manual so I
put a reference to it in a comment.

> The var HAL code should not call mace1 specific functions. How you have debug
> code in hal_IRQ_handler which does. This code needs removing, or you need to us
> a generic LED function which all MAC7100 have to implement.
>   

I intended to remove it sooner or later, just after little bit more testing (and
eventual user feedback). Now it is removed.

> In a couple of places i found the name MACE0. How is this related to MACE1?
> Should all the MACE0 be changed to MACE1?
>   

I forgot to change, sorry. MACE0 was MACE1's (MAC Eval board 1) working name MACE1.

> _led_init: Why do you have all the strh commands? If there is a real reason for
> this a comment would be good.
>   

That's simply MAC I/O configuration feature. Every pin has it's own register.
I was thinking of putting port init. in a loop, but it would require another
register.
At the time I was beginning the port I knew very little about eCos booting
process (as well as about eCos itself - actually LED macros were my 1st ever
eCos port code) and since it states,  in vectors.S, that LED macro can use only
r0 and r1 I decided to play safe and make stupidest code. I you wish I can loop
it, but I think that space gain isn't worth it.

On the other hand now I have degenerated macro _pio_init to nothing since I have
implemented initialization of serial ports' pins in C.

> The CDL code contains ROM,RAM & ROMRAM startup types. But you only have ROM
> linker files etc. Please add the missing files, or remove the unused CDL.
>   

MACE1 hasn't external memory so RAM and ROM/RAM doesn't have much sense so I
place following:

cdl_component CYG_HAL_STARTUP {
       display       "Startup type"
       flavor        data
       calculated {"ROM"}
       #default_value {"ROM"}
       #legal_values  {"RAM"  "ROM" "ROMRAM"}
       no_define
       define -file system.h CYG_HAL_STARTUP
       description   "
           When targetting the MAC7100 MACE1 eval board it is possible to build
           the system for either RAM bootstrap or ROM bootstrap(s). Select
           'ram' when building programs to load into RAM using onboard
           debug software such as Angel or eCos GDB stubs.  Select 'rom'
           when building a stand-alone application which will be put
           into ROM.  Using ROMRAM will allow the program to exist in
           ROM, but be copied to RAM during startup."
   }

the alternative would be to delete        calculated {"ROM"}
and leave:
 ...........
       default_value {"ROM"}
       legal_values  {"RAM"  "ROM" "ROMRAM"}
 ..........

please advise.

> I suggest moving the serial port driver header file into /cyg/dev/ser_esci.h
>   

done. I actually state devs instead of dev to reflect repository dir name.

> Please supply patches to the existing code, not a complete new patch. I've made
> some changes which i want to keep and using patches makes this possible.   

I applied all suggested changes changes more or less. I would send patch only I
have there dillemae about 1st and last issue i.e.

"requires { CYGPKG_IO_SERIAL_DEVICES == 1 }" and  CDL code for ROMRAM.

Please send me your advice so I can prepare the patch.

Best regards
Ilija





-- 
Configure bugmail: http://bugzilla.ecoscentric.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.