[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-ppc] [PATCH 04/10] sam460ex: Don't size flash mem
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [Qemu-ppc] [PATCH 04/10] sam460ex: Don't size flash memory to match backing image |
Date: |
Mon, 18 Feb 2019 18:57:29 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
BALATON Zoltan <address@hidden> writes:
> Hello,
>
> On Mon, 18 Feb 2019, Markus Armbruster wrote:
>> Machine "sam460ex" maps its flash memory at address 0xFFF00000. When
>> no image is supplied, its size is 1MiB (0x100000). Else, it's the
>> size of the image rounded up to the next multiple of 64KiB.
>>
>> The rounding is actually useless: pflash_cfi01_realize() fails with
>> "failed to read the initial flash content" unless it's a no-op.
>>
>> I have no idea what happens when the pflash's size exceeds 1MiB.
>> Useful outcomes seem unlikely.
>>
>> I guess memory at the end of the address space remains unmapped when
>> it's smaller than 1MiB. Again, useful outcomes seem unlikely.
>
> I'm not sure where this was coming from but it predates my changes so
> no idea either.
>
>> Set the flash memory size to 1MiB regardless of image size, to match
>> the physical hardware.
>
> Actually the real hardware seems to have a 512 kB flash chip:
> https://eu.mouser.com/datasheet/2/268/atmel_AT49BV040B-1180330.pdf
Fascinating.
<http://www.sam4x0.com/sam460ex.html> confirms.
> so while you're at it you could change FLASH_SIZE to match that.
Leads to more questions, below.
>> Cc: BALATON Zoltan <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>> hw/ppc/sam460ex.c | 23 ++++++++---------------
>> 1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 75250d49e4..ca8d7ab9c6 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -92,31 +92,24 @@ struct boot_info {
>> static int sam460ex_load_uboot(void)
>> {
>> DriveInfo *dinfo;
>> - BlockBackend *blk = NULL;
>> - hwaddr base = FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32);
>> - long bios_size = FLASH_SIZE;
>> - int fl_sectors;
>>
>> dinfo = drive_get(IF_PFLASH, 0, 0);
>> - if (dinfo) {
>> - blk = blk_by_legacy_dinfo(dinfo);
>> - bios_size = blk_getlength(blk);
>
> After this maybe the
> #include "sysemu/block-backend.h"
> can also be dropped from the includes?
I'll try.
>> - }
>> - fl_sectors = (bios_size + 65535) >> 16;
>> -
>> - if (!pflash_cfi01_register(base, NULL, "sam460ex.flash", bios_size,
>> - blk, 64 * KiB, fl_sectors,
>> + if (!pflash_cfi01_register(FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32),
>> + NULL, "sam460ex.flash", FLASH_SIZE,
>> + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>> + 65536, FLASH_SIZE / 65536,
>
> Could you keep 64 * KiB instead of 65536 here to match other places?
Sure (editing accident).
> Regards,
> BALATON Zoltan
>
>> 1, 0x89, 0x18, 0x0000, 0x0, 1)) {
>> error_report("Error registering flash memory");
>> /* XXX: return an error instead? */
>> exit(1);
>> }
>>
>> - if (!blk) {
>> + if (!dinfo) {
>> /*error_report("No flash image given with the 'pflash' parameter,"
>> " using default u-boot image");*/
>> - base = UBOOT_LOAD_BASE | ((hwaddr)FLASH_BASE_H << 32);
>> - rom_add_file_fixed(UBOOT_FILENAME, base, -1);
>> + rom_add_file_fixed(UBOOT_FILENAME,
>> + UBOOT_LOAD_BASE | ((hwaddr)FLASH_BASE_H << 32),
>> + -1);
>> }
>>
>> return 0;
Let's have a look at the resulting function:
static int sam460ex_load_uboot(void)
{
DriveInfo *dinfo;
dinfo = drive_get(IF_PFLASH, 0, 0);
if (!pflash_cfi01_register(FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32),
"sam460ex.flash", FLASH_SIZE,
dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
65536,
1, 0x89, 0x18, 0x0000, 0x0, 1)) {
error_report("Error registering flash memory");
/* XXX: return an error instead? */
exit(1);
}
if (!dinfo) {
/*error_report("No flash image given with the 'pflash' parameter,"
" using default u-boot image");*/
rom_add_file_fixed(UBOOT_FILENAME,
UBOOT_LOAD_BASE | ((hwaddr)FLASH_BASE_H << 32),
-1);
}
return 0;
}
This first creates 1MiB of flash memory mapped at the end of the 32-bit
address space (0xFFF00000..0xFFFFFFFF).
If_PFLASH unit 0 is defined, the flash memory is initialized from that
block backend.
Else, it's initialized to zero. And then 512KiB of ROM gets mapped on
top of its second half (0xFFF80000..0xFFFFFFFF), initialized from
u-boot-sam460-20100605.bin (which we build).
This doesn't smell right.
I propose to do the following: if IF_PFLASH unit 0 is defined, create
512KiB of flash memory mapped at the end of the 32-bit address space,
else, create 512KiB of ROM there.
Okay?
- Re: [Qemu-devel] [PATCH 02/10] pflash: Macro PFLASH_BUG() is used just once, expand, (continued)
[Qemu-devel] [PATCH 10/10] hw/arm hw/xtensa: De-duplicate pflash creation code some, Markus Armbruster, 2019/02/18
[Qemu-devel] [PATCH 04/10] sam460ex: Don't size flash memory to match backing image, Markus Armbruster, 2019/02/18
[Qemu-devel] [PATCH 06/10] r2d: Flash memory creation is confused about size, mark FIXME, Markus Armbruster, 2019/02/18
Re: [Qemu-devel] [PATCH 06/10] r2d: Flash memory creation is confused about size, mark FIXME, Philippe Mathieu-Daudé, 2019/02/19