From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41464) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XNQPS-00071J-4u for qemu-devel@nongnu.org; Fri, 29 Aug 2014 13:56:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XNQPM-0005jp-0N for qemu-devel@nongnu.org; Fri, 29 Aug 2014 13:56:26 -0400 Received: from mail.uni-paderborn.de ([131.234.142.9]:53651) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XNQPL-0005jW-ME for qemu-devel@nongnu.org; Fri, 29 Aug 2014 13:56:19 -0400 Message-ID: <5400CDC7.1030601@mail.uni-paderborn.de> Date: Fri, 29 Aug 2014 20:00:23 +0100 From: Bastian Koppelmann MIME-Version: 1.0 References: <1408726356-14420-1-git-send-email-kbastian@mail.uni-paderborn.de> <1408726356-14420-3-git-send-email-kbastian@mail.uni-paderborn.de> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 02/15] target-tricore: Add board for systemmode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , Richard Henderson Hi Peter, On 08/29/2014 03:30 PM, Peter Maydell wrote: > >> + >> + dinfo = drive_get(IF_PFLASH, 0, 0); >> + if (!pflash_cfi01_register(TRICORE_FLASH_ADDR, NULL, >> + "tricore_testboard.flash", >> + TRICORE_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL, >> + TRICORE_FLASH_SECT_SIZE, >> + TRICORE_FLASH_SIZE / TRICORE_FLASH_SECT_SIZE, >> + 2, 0x00, 0x00, 0x0000, 0x0, 0)) { > Don't use pflash_cfi01_register() in new code, it gives you a > weird not-like-hardware flash device that we only have for > backwards compatibility with existing board models. Instantiate > and configure the flash device directly (compare vexpress.c's > ve_pflash_cfi01_register(), but use the correct config for the > hardware you're modelling, which might not be two parallel > 16 bit wide flash chips). This board does not exists. It's purpose is just to be able to run TriCore binaries. So I rather just get rid of the flash drive. >> + } >> + >> + tricoretb_binfo.ram_size = machine->ram_size; >> + tricoretb_binfo.kernel_filename = machine->kernel_filename; >> + >> + if (machine->kernel_filename) { >> + tricore_load_kernel(env); >> + } >> +} >> + >> +static void tricoreboard_init(MachineState *machine) >> +{ >> + tricore_testboard_init(machine, 0x183); >> +} >> + >> +static QEMUMachine ttb_machine = { >> + .name = "tricore_testboard", >> + .desc = "Just for testing", >> + .init = tricoreboard_init, >> + .is_default = 1, >> +}; > The .desc text here appears in the user-visible help output, so > can we have something slightly more useful to them, please? > > $ ./build/all/tricore-softmmu/qemu-system-tricore -M help > Supported machines are: > tricore_testboard Just for testing (default) > none empty machine Same as above. The description shows exactly the board's purpose. It is just for testing and not a real board. How about I change it to "a minimal TriCore board"? I see your point about the default. For now I would leave it as is, since there are not other boards and qemu would give an error, when launched without -M. > Also, think about whether you really want to set the > is_default flag. Will all users of TriCore based boards > always want the test board? > >> +++ b/include/hw/tricore/tricore.h >> @@ -0,0 +1,54 @@ >> +#ifndef TRICORE_MISC_H >> +#define TRICORE_MISC_H 1 >> + >> +#include "exec/memory.h" >> +#include "hw/irq.h" >> + >> +struct tricore_boot_info { >> + uint64_t ram_size; >> + const char *kernel_filename; >> + const char *kernel_cmdline; >> + const char *initrd_filename; >> + const char *dtb_filename; >> + hwaddr loader_start; >> + /* multicore boards that use the default secondary core boot functions >> + * need to put the address of the secondary boot code, the boot reg, >> + * and the GIC address in the next 3 values, respectively. boards that >> + * have their own boot functions can use these values as they want. >> + */ >> + hwaddr smp_loader_start; >> + hwaddr smp_bootreg_addr; >> + hwaddr gic_cpu_if_addr; >> + int nb_cpus; >> + int board_id; >> + int (*atag_board)(const struct tricore_boot_info *info, void *p); >> + /* multicore boards that use the default secondary core boot functions >> + * can ignore these two function calls. If the default functions won't >> + * work, then write_secondary_boot() should write a suitable blob of >> + * code mimicking the secondary CPU startup process used by the board's >> + * boot loader/boot ROM code, and secondary_cpu_reset_hook() should >> + * perform any necessary CPU reset handling and set the PC for the >> + * secondary CPUs to point at this boot blob. >> + */ >> + void (*write_secondary_boot)(TRICORECPU *cpu, >> + const struct tricore_boot_info *info); >> + void (*secondary_cpu_reset_hook)(TRICORECPU *cpu, >> + const struct tricore_boot_info *info); >> + /* if a board is able to create a dtb without a dtb file then it >> + * sets get_dtb. This will only be used if no dtb file is provided >> + * by the user. On success, sets *size to the length of the created >> + * dtb, and returns a pointer to it. (The caller must free this memory >> + * with g_free() when it has finished with it.) On failure, returns NULL. >> + */ >> + void *(*get_dtb)(const struct tricore_boot_info *info, int *size); >> + /* if a board needs to be able to modify a device tree provided by >> + * the user it should implement this hook. >> + */ >> + void (*modify_dtb)(const struct tricore_boot_info *info, void *fdt); >> + /* Used internally by arm_boot.c */ >> + int is_linux; >> + hwaddr initrd_start; >> + hwaddr initrd_size; >> + hwaddr entry; >> +}; > This looks pretty obviously like you just copied it wholesale from > the ARM board support code. Please only include struct fields > you actually use. > > thanks > -- PMM > Thanks, Bastian