Re: [Qemu-ppc] [Qemu-devel] [PATCH 05/10] ppc405_boards: Don't size flash memory to match backing image

[Markus Armbruster]

Alex Bennée <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Machine "ref405ep" maps its flash memory at address 2^32 - image size.
>> Image size is rounded up to the next multiple of 64KiB.  Useless,
>> because pflash_cfi02_realize() fails with "failed to read the initial
>> flash content" unless the rounding is a no-op.
>>
>> If the image size exceeds 0x80000 Bytes, we overlap first SRAM, then
>> other stuff.  No idea how that would play out, but a useful outcomes
>> seem unlikely.
>>
>> Map the flash memory at fixed address 0xFFF80000 with size 512KiB,
>> regardless of image size, to match the physical hardware.
>>
>> Machine "taihu" maps its boot flash memory similarly.  The code even
>> has a comment /* XXX: should check that size is 2MB */, followed by
>> disabled code to adjust the size to 2MiB regardless of image size.
>>
>> Its code to map its application flash memory looks the same, except
>> there the XXX comment asks for 32MiB, and the code to adjust the size
>> isn't disabled.  Note that pflash_cfi02_realize() fails with "failed
>> to read the initial flash content" for images smaller than 32MiB.
>>
>> Map the boot flash memory at fixed address 0xFFE00000 with size 2MiB,
>> to match the physical hardware.  Delete dead code from application
>> flash mapping, and simplify some.
>>
>> Cc: David Gibson <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/ppc/ppc405_boards.c | 53 +++++++++++++-----------------------------
>>  1 file changed, 16 insertions(+), 37 deletions(-)
>>
>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>> index f47b15f10e..728154aebb 100644
>> --- a/hw/ppc/ppc405_boards.c
>> +++ b/hw/ppc/ppc405_boards.c
>> @@ -158,7 +158,7 @@ static void ref405ep_init(MachineState *machine)
>>      target_ulong kernel_base, initrd_base;
>>      long kernel_size, initrd_size;
>>      int linux_boot;
>> -    int fl_idx, fl_sectors, len;
>> +    int len;
>>      DriveInfo *dinfo;
>>      MemoryRegion *sysmem = get_system_memory();
>>
>> @@ -185,26 +185,19 @@ static void ref405ep_init(MachineState *machine)
>>  #ifdef DEBUG_BOARD_INIT
>>      printf("%s: register BIOS\n", __func__);
>>  #endif
>> -    fl_idx = 0;
>>  #ifdef USE_FLASH_BIOS
>> -    dinfo = drive_get(IF_PFLASH, 0, fl_idx);
>> +    dinfo = drive_get(IF_PFLASH, 0, 0);
>>      if (dinfo) {
>> -        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> -
>> -        bios_size = blk_getlength(blk);
>> -        fl_sectors = (bios_size + 65535) >> 16;
>>  #ifdef DEBUG_BOARD_INIT
>> -        printf("Register parallel flash %d size %lx"
>> -               " at addr %lx '%s' %d\n",
>> -               fl_idx, bios_size, -bios_size,
>> -               blk_name(blk), fl_sectors);
>> +        printf("Register parallel flash\n");
>>  #endif
>> -        pflash_cfi02_register((uint32_t)(-bios_size),
>> +        bios_size = 0x80000;
>
>  bios_size = 8 * MiB?

The next line has base address 0xFFF80000.  I picked 0x80000 to make
0xFFF80000 + 0x80000 == 0 mod 2^32 more obvious.

If I change 0x80000 to 8 * MiB, the size is more obvious, but "at end of
32 bit address space" less so.

If I additionally change the base address back to ((uint32_t)-bios_size,
"at end of 32 bit address space" is obvious again, but the actual base
address less so.

I don't really care myself.  David, you're the maintainer, do you have a
preference?

>> +        pflash_cfi02_register(0xFFF80000,
>>                                NULL, "ef405ep.bios", bios_size,
>> -                              blk, 65536, fl_sectors, 1,
>> +                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>> +                              65536, bios_size / 65536, 1,
>
> 64 * KiB?

David, same question (two additional instances below).

>>                                2, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 
>> 0x2AA,
>>                                1);
>> -        fl_idx++;
>>      } else
>>  #endif
>>      {
>> @@ -455,7 +448,7 @@ static void taihu_405ep_init(MachineState *machine)
>>      target_ulong kernel_base, initrd_base;
>>      long kernel_size, initrd_size;
>>      int linux_boot;
>> -    int fl_idx, fl_sectors;
>> +    int fl_idx;
>>      DriveInfo *dinfo;
>>
>>      /* RAM is soldered to the board so the size cannot be changed */
>> @@ -486,21 +479,14 @@ static void taihu_405ep_init(MachineState *machine)
>>  #if defined(USE_FLASH_BIOS)
>>      dinfo = drive_get(IF_PFLASH, 0, fl_idx);
>>      if (dinfo) {
>> -        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> -
>> -        bios_size = blk_getlength(blk);
>> -        /* XXX: should check that size is 2MB */
>> -        //        bios_size = 2 * 1024 * 1024;
>> -        fl_sectors = (bios_size + 65535) >> 16;
>>  #ifdef DEBUG_BOARD_INIT
>> -        printf("Register parallel flash %d size %lx"
>> -               " at addr %lx '%s' %d\n",
>> -               fl_idx, bios_size, -bios_size,
>> -               blk_name(blk), fl_sectors);
>> +        printf("Register boot flash\n");
>>  #endif
>> -        pflash_cfi02_register((uint32_t)(-bios_size),
>> +        bios_size = 2 * MiB;
>> +        pflash_cfi02_register(0xFFE00000,
>>                                NULL, "taihu_405ep.bios", bios_size,
>> -                              blk, 65536, fl_sectors, 1,
>> +                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>> +                              65536, bios_size / 65536, 1,
>
> 64 * KiB
>
>>                                4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 
>> 0x2AA,
>>                                1);
>>          fl_idx++;
>> @@ -536,20 +522,13 @@ static void taihu_405ep_init(MachineState *machine)
>>      /* Register Linux flash */
>>      dinfo = drive_get(IF_PFLASH, 0, fl_idx);
>>      if (dinfo) {
>> -        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> -
>> -        bios_size = blk_getlength(blk);
>> -        /* XXX: should check that size is 32MB */
>>          bios_size = 32 * MiB;
>> -        fl_sectors = (bios_size + 65535) >> 16;
>>  #ifdef DEBUG_BOARD_INIT
>> -        printf("Register parallel flash %d size %lx"
>> -               " at addr " TARGET_FMT_lx " '%s'\n",
>> -               fl_idx, bios_size, (target_ulong)0xfc000000,
>> -               blk_name(blk));
>> +        printf("Register application flash\n"
>>  #endif
>>          pflash_cfi02_register(0xfc000000, NULL, "taihu_405ep.flash", 
>> bios_size,
>> -                              blk, 65536, fl_sectors, 1,
>> +                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>> +                              65536, bios_size / 65536, 1,
>
> 64 * KiB
>
>>                                4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 
>> 0x2AA,
>>                                1);
>>          fl_idx++;
>
>
> --
> Alex Bennée