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.kono...@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
