Re: [Qemu-devel] [PATCH v6 02/15] target-tricore: Add board for systemmode

[Bastian Koppelmann <kbastian@mail.uni-paderborn.de>]

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