Re: [Qemu-devel] [PATCH 2/2] versatiblepb: add NOR flash support

[Peter Maydell]

On 12 April 2012 08:51, Eric Bénard <address@hidden> wrote:
> - add support for the 64MB NOR CFI01 flash available at
> 0x34000000 on the versatilepb board
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0225d/BBAJIHEC.html
>
> - give the possibility to boot from the flash if no kernel
> is provided to qemu
>
> - initial idea from
> http://lists.gnu.org/archive/html/qemu-devel/2008-10/msg00350.html
>
> - tested with barebox bootloader
>
> Signed-off-by: Eric Bénard <address@hidden>

This patch has a number of style issues which scripts/checkpatch.pl
complains about. Please fix them. However, you have a larger problem...

> -    versatile_binfo.ram_size = ram_size;
> -    versatile_binfo.kernel_filename = kernel_filename;
> -    versatile_binfo.kernel_cmdline = kernel_cmdline;
> -    versatile_binfo.initrd_filename = initrd_filename;
> -    versatile_binfo.board_id = board_id;
> -    arm_load_kernel(env, &versatile_binfo);
> +    if (kernel_filename) {
> +        versatile_binfo.ram_size = ram_size;
> +        versatile_binfo.kernel_filename = kernel_filename;
> +        versatile_binfo.kernel_cmdline = kernel_cmdline;
> +        versatile_binfo.initrd_filename = initrd_filename;
> +        versatile_binfo.board_id = board_id;
> +        arm_load_kernel(env, &versatile_binfo);
> +    } else if (fl_idx) {
> +        env->regs[15] = VERSATILE_FLASH_ADDR;

This is the wrong way to do this -- the board should not be randomly modifying
the PC like this. (It's a layering violation and it won't work correctly if
the model is reset either.) You need to model the way the real hardware
supports starting with addresses 0..0x03FFFFFF remapped to be an alias of
the flash.

Unfortunately that means you'll need to implement at least a minimal model
of the SP810 system controller, because that's what controls the remapping.

-- PMM