Re: [PATCH v2 1/1] tricore: added triboard with tc27x_soc

[Bastian Koppelmann]

Hi,

thanks for the patch. In general this looks good to me. However, a have a few
nitpicks.

On Tue, Jun 09, 2020 at 05:25:53PM +0200, David Brenken wrote:
> From: Andreas Konopik <andreas.konopik@efs-auto.de>
> +static const int tc27x_soc_irqmap[] = {
> +};

Since this is empty, it's best to just remove it.

> +
> +static const hwaddr tc27x_soc_memmap[] = {
> +    [TC27XD_DSPR2]     = 0x50000000,
> +    [TC27XD_DCACHE2]   = 0x5001E000,
> +    [TC27XD_DTAG2]     = 0x500C0000,
> +    [TC27XD_PSPR2]     = 0x50100000,
> +    [TC27XD_PCACHE2]   = 0x50108000,
> +    [TC27XD_PTAG2]     = 0x501C0000,
> +    [TC27XD_DSPR1]     = 0x60000000,
> +    [TC27XD_DCACHE1]   = 0x6001E000,
> +    [TC27XD_DTAG1]     = 0x600C0000,
> +    [TC27XD_PSPR1]     = 0x60100000,
> +    [TC27XD_PCACHE1]   = 0x60108000,
> +    [TC27XD_PTAG1]     = 0x601C0000,
> +    [TC27XD_DSPR0]     = 0x70000000,
> +    [TC27XD_PSPR0]     = 0x70100000,
> +    [TC27XD_PCACHE0]   = 0x70106000,
> +    [TC27XD_PTAG0]     = 0x701C0000,
> +    [TC27XD_PFLASH0_C] = 0x80000000,
> +    [TC27XD_PFLASH1_C] = 0x80200000,
> +    [TC27XD_OLDA_C]    = 0x8FE70000,
> +    [TC27XD_BROM_C]    = 0x8FFF8000,
> +    [TC27XD_LMURAM_C]  = 0x90000000,
> +    [TC27XD_EMEM_C]    = 0x9F000000,
> +    [TC27XD_PFLASH0_U] = 0xA0000000,
> +    [TC27XD_PFLASH1_U] = 0xA0200000,
> +    [TC27XD_DFLASH0]   = 0xAF000000,
> +    [TC27XD_DFLASH1]   = 0xAF110000,
> +    [TC27XD_OLDA_U]    = 0xAFE70000,
> +    [TC27XD_BROM_U]    = 0xAFFF8000,
> +    [TC27XD_LMURAM_U]  = 0xB0000000,
> +    [TC27XD_EMEM_U]    = 0xBF000000,
> +    [TC27XD_PSPRX]     = 0xC0000000,
> +    [TC27XD_DSPRX]     = 0xD0000000,
> +};

Can we add the sizes here as well? That make it much easier to read. See
hw/riscv/sifive_e.c

Also what do the _U and _C suffixes mean? I could not find them in the user
manual [1].

> +
> +/*
> + * Initialize the auxiliary ROM region @mr and map it into
> + * the memory map at @base.
> + */
> +static void make_rom(MemoryRegion *mr, const char *name,
> +                     hwaddr base, hwaddr size)
> +{
> +    memory_region_init_rom(mr, NULL, name, size, &error_fatal);
> +    memory_region_add_subregion(get_system_memory(), base, mr);
> +}
> +
> +/*
> + * Initialize the auxiliary RAM region @mr and map it into
> + * the memory map at @base.
> + */
> +static void make_ram(MemoryRegion *mr, const char *name,
> +                     hwaddr base, hwaddr size)
> +{
> +    memory_region_init_ram(mr, NULL, name, size, &error_fatal);
> +    memory_region_add_subregion(get_system_memory(), base, mr);
> +}
> +
> +/*
> + * Create an alias of an entire original MemoryRegion @orig
> + * located at @base in the memory map.
> + */
> +static void make_alias(MemoryRegion *mr, const char *name,
> +                           MemoryRegion *orig, hwaddr base)
> +{
> +    memory_region_init_alias(mr, NULL, name, orig, 0,
> +                             memory_region_size(orig));
> +    memory_region_add_subregion(get_system_memory(), base, mr);
> +}

These seem like very common idioms. It might be worth while to make this a
generic QEMU API. However this is out of scope for this patchset.

> +    /*
> +     * TriCore QEMU executes CPU0 only, thus it is sufficient to map
> +     * LOCAL.PSPR/LOCAL.DSPR exclusively onto PSPR0/DSPR0.
> +     */
> +    make_alias(&s->psprX, "LOCAL.PSPR", &s->cpu0mem.pspr,
> +            sc->memmap[TC27XD_PSPRX]);
> +    make_alias(&s->dsprX, "LOCAL.DSPR", &s->cpu0mem.dspr,
> +            sc->memmap[TC27XD_DSPRX]);
  
These aliases point to reserved memory in the user manual [1].

> +static void tc27x_soc_init(Object *obj)
> +{
> +    TC27XSoCState *s = TC27X_SOC(obj);
> +    TC27XSoCClass *sc = TC27X_SOC_GET_CLASS(s);
> +
> +    sysbus_init_child_obj(OBJECT(s), "tc27x", OBJECT(&s->cpu), 
> sizeof(s->cpu),
> +                        sc->cpu_type);

Unnecessary cast. Just use sysbus_init_child_obj(obj,...)

> +static void tricore_load_kernel(const char *kernel_filename)
> +{
> +    uint64_t entry;
> +    long kernel_size;
> +    TriCoreCPU *cpu;
> +    CPUTriCoreState *env;
> +
> +    kernel_size = load_elf(kernel_filename, NULL,
> +                           NULL, NULL, &entry, NULL,
> +                           NULL, NULL, 0,
> +                           EM_TRICORE, 1, 0);
> +    if (kernel_size <= 0) {
> +        error_report("no kernel file '%s'", kernel_filename);
> +        exit(1);
> +    }
> +    cpu = TRICORE_CPU(first_cpu);
> +    env = &cpu->env;
> +    env->PC = entry;
> +}

Just a note for the future. This seems like a function that ought to be
generalized for all TriCore boards.

Cheers,
Bastian

[1] 
https://hitex.co.uk/fileadmin/uk-files/downloads/ShieldBuddy/tc27xD_um_v2.2.pdf