Re: [avr-gcc-list] Patch avrtest: add support for avr6

[Paulo Marques]

Tristan Gingold wrote:
> Hi,

Hi, Tristan

> it has been a while since I didn't update my patch.
> Here is the new version.  It adds a new switch: -mmcu=X, where X can be 
> avr51 (default) or avr6.

Nice :)

> The switch selects an architecture which describes features of the 
> processor.
> The maximum memory size now depends on the architecture.

Overall it seems fine, but I have a few comments (see below)

> Index: avrtest.c
> ===================================================================
> RCS file: /cvsroot/winavr/avrtest/avrtest.c,v
> retrieving revision 1.7
> diff -c -r1.7 avrtest.c
> *** avrtest.c   5 Jun 2008 04:07:27 -0000       1.7
> --- avrtest.c   17 Jun 2008 20:18:02 -0000
> ***************
> *** 29,38 ****
>   #include <stdint.h>
>   
>   // ---------------------------------------------------------------------------------
> ! //     configuration values
>   
>   #define MAX_RAM_SIZE	 64 * 1024
> ! #define MAX_FLASH_SIZE  256 * 1024
>   
>   // ---------------------------------------------------------------------------------
>   //     register and port definitions
> --- 29,38 ----
>   #include <stdint.h>
>   
>   // ---------------------------------------------------------------------------------
> ! //     configuration values (in bytes).
>   
>   #define MAX_RAM_SIZE	 64 * 1024
> ! #define MAX_FLASH_SIZE  256 * 1024    // Must be at least 128KB
>   
>   // ---------------------------------------------------------------------------------
>   //     register and port definitions
> ***************
> *** 63,68 ****
> --- 63,88 ----
>   #define FLAG_Z        0x02
>   #define FLAG_C        0x01
>   
> + struct arch_desc {
> +       // Name of the architecture.
> +       const char *name;
> +       // True if PC is 3 bytes, false if only 2 bytes.
> +       unsigned int pc_3bytes : 1;
> +       // True if the architecture has EIND related insns (EICALL/EIJMP).
> +       unsigned int has_eind : 1;
> + };

I don't know if it makes sense to use bit fields here, because there 
might be a performance penalty in accessing them and "pc_3bytes" is 
accessed a few times while executing avr code.

> + // List of supported archs with their features.
> + const struct arch_desc arch_descs[] =
> + {
> +       { "avr51", 0, 0},
> +       { "avr6", 1, 1},
> +       { NULL, 0, 0}
> + };

It would probably bew easier to read these structures if they were 
written like:

const struct arch_desc arch_descs[] = {
  {
    .name = "avr51",
    .pc_3_bytes = 0,
    .has_eind = 0,
  },
  {
    .name = "avr6",
    .pc_3_bytes = 1,
    .has_eind = 1,
  },
  { NULL, 0, 0}
};

I know its more verbose, but if we keep adding flags it will become 
impossible to understand what { "avr51", 0, 0, 1, 0, 1, 1, 0 } means ;)

> + const struct arch_desc *arch = &arch_descs[0];
> + 
> + 

[...]

> --- 1576,1631 ----
>   {
>         int i;
>   
>   	//max_instr_count = 1000000000;
>         max_instr_count = 0;
>   
>   	// parse command line arguments
>         for (i = 1; i < argc; i++) {
> !               //  Use naive but very portable method to decode arguments.
> !               if (strcmp(argv[i], "-d") == 0) {
> !                       flag_initialize_sram = 1;
> !               }
> !               else if (strcmp(argv[i], "-m") == 0) {
> !                       i++;
> !                       if (i >= argc)
> !                               usage();
> !                       max_instr_count = strtoul(argv[i], NULL, 0);
> !               }
> !               else if (strncmp(argv[i], "-mmcu=", 6) == 0) {
> !                       const struct arch_desc *d;
> !                       for (d = arch_descs; d->name; d++)
> !                               if (strcmp(argv[i] + 6, d->name) == 0) {
> !                                       arch = d;
> !                                       break;
> !                               }
> !                       if (d->name == NULL)
>                                 usage();

You forgot to update usage() to reflect the new options, no?

>                 }
>                 else {
> !                       if (program_name != NULL)
>                                 usage();
> +                       load_to_flash(argv[i]);
> +                       // Must be the last argument.
> +                       if (i + 1 != argc)
> +                               usage ();

I don't think we need to restrict the program name to be the last argument.

>                 }
>         }
>   
> ! 	// setup default values
> !       flash_addr_mask = (arch->pc_3bytes ? MAX_FLASH_SIZE : (1 << 16)) - 1;

flash_addr_mask is in bytes, so to keep the previous default 128kB this 
should be "(1 << 17) - 1", no?

> [...]

Anyway, overall this seems like a nice simple structure to add more 
architectures and features in the future.

--
Paulo Marques
Software Development Department - Grupo PIE, S.A.
Phone: +351 252 290600, Fax: +351 252 290601
Web: www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."