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

[Bastian Koppelmann]

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