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

[Konopik, Andreas (EFS-GH2)]

Hello Bastian,

I assume that your review comments, which Georg has answered, are solved. I 
will implement the remaining suggestions in a new version of the patch.

Regards,

Andreas

> -----Ursprüngliche Nachricht-----
> Von: Hofstetter, Georg (EFS-GH2) <Georg.Hofstetter@efs-auto.de>
> Gesendet: Mittwoch, 10. Juni 2020 15:50
> An: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: qemu-devel@nongnu.org; Konopik, Andreas (EFS-GH2)
> <andreas.konopik@efs-auto.de>; Brenken, David (EFS-GH5)
> <david.brenken@efs-auto.de>
> Betreff: AW: [PATCH v2 1/1] tricore: added triboard with tc27x_soc
> 
> Hello Bastian,
> 
> Thanks for your feedback, I like your proposals.
> 
> > Also what do the _U and _C suffixes mean? I could not find them in the user
> manual [1].
> 
> See TC27X UM chapter 3.2 "Contents of the Segments"
> "CPUx default attributes for these segments: non-cached" vs "cached"
> These regions are the same memory, just with address bit 29 set, which will
> bypass the DMI/PMI cache.
> The regions are reflected here with _C (cached) and _U (uncached) suffixes.
> 
> > These aliases point to reserved memory in the user manual [1].
> 
> See TC27X UM chapter 5.7.2 "Local and Global Addressing"
> "The local PSPR memory is always located at C0000000H. The local DSPR is
> always located at D0000000H."
> Those addresses are not visible on the SRI crossbar, but are core-locally 
> mapped
> onto the current core memories.
> As we emulate only one core (yet?), the "local DSPR" is being mapped to "CPU0
> DSPR", same for PSPR.
> 
> 
> Regards,
> Georg
> 
> 
> -----Ursprüngliche Nachricht-----
> Von: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Gesendet: Mittwoch, 10. Juni 2020 10:49
> An: David Brenken <david.brenken@efs-auto.org>
> Cc: qemu-devel@nongnu.org; Konopik, Andreas (EFS-GH2)
> <andreas.konopik@efs-auto.de>; Brenken, David (EFS-GH5)
> <david.brenken@efs-auto.de>; Hofstetter, Georg (EFS-GH2)
> <Georg.Hofstetter@efs-auto.de>; Rasche, Robert (EFS-GH2)
> <robert.rasche@efs-auto.de>; Biermanski, Lars (EFS-GH3)
> <lars.biermanski@efs-auto.de>
> Betreff: Re: [PATCH v2 1/1] tricore: added triboard with tc27x_soc
> 
> 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