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

[Markus Armbruster <armbru@redhat.com>]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Mon, 18 Feb 2019, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> 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.
[...]
>> 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.
>
> Unfortunately I don't know much about how this should work. Maybe
> François can remember where this comes from, this was already there
> when I started working on it, but I suspect maybe he's copied it from
> some other board in QEMU as well. The sam460ex was based on
> ppc440_bamboo but that does not seem to have flash ROM. Memory SPD
> EEPROM came from mips_malta (cleaned up since but it shows that that
> was also used as inspiration) but that's also not the same so maybe it
> was adapted for sam460ex or came from some other example in QEMU. This
> was already there in François's initial commit:
>
> https://github.com/mmuman/qemu/commit/d10cc631645f3893d53e60cc00c618470b4de52c#diff-73d06ebbc1301aab78105d853097fa2fR42
>
> and later was slightly modified (maybe to rebase for changes in QEMU):
>
> https://github.com/mmuman/qemu/commit/768136b08a6b9b69e707af2c478b68a5935bb8f0#diff-73d06ebbc1301aab78105d853097fa2fL1267
>
> The comment says these values come from U-Boot:
>
> https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=include/configs/Sam460ex.h;h=71064a9601c89dd3ce381d07e0def6c9d5294d44;hb=HEAD#l123
>
> and that indeed has flash size of 1 MB but then builds an image that's
> exactly 512 kB which should be mapped at end of 4GB because the
> initial program counter of the CPU is 0xfffffffc and board has a 512kB
> flash chip as well.
>
>> 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?
>
> AFAIU the above U-Boot could handle up to 1MB of ROM but board has a
> 512kB chip so probably it makes sense to use 512k here. However since
> this is not well understood (at least by me) I'm not asking you to do
> that and maybe just leave it as it is now. This can be revisited when
> NVRAM is implemented later as this may be related to that (or not)
> this would need understanding of some details I don't have yet. But if
> you feel confident enough to clean this up feel free to go ahead.

Then let's use my patch as is, plus a FIXME comment explaining the
situation.  Okay?