[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
AW: [PATCH v2 1/1] tricore: added triboard with tc27x_soc
|
From: |
Hofstetter, Georg (EFS-GH2) |
|
Subject: |
AW: [PATCH v2 1/1] tricore: added triboard with tc27x_soc |
|
Date: |
Wed, 10 Jun 2020 13:49:37 +0000 |
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