On 3/6/19 7:51 AM, Markus Armbruster wrote: > BALATON Zoltan <bala...@eik.bme.hu> writes: >> On Tue, 5 Mar 2019, Philippe Mathieu-Daudé wrote: >>> On 2/26/19 8:34 PM, Markus Armbruster wrote: >>>> Machine "sam460ex" maps its flash memory at address 0xFFF00000. When >>>> no image is supplied, its size is 1MiB (0x100000), and 512KiB of ROM >>>> get mapped on top of its second half. 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. >>> >>> With PFlashCFI02, it depends of the @nb_mappings parameter, which tries >>> to emulates how the bus connects the pflash (which address lines are >>> connected). >>> >>> PFlashCFI01 doesn't support the feature to remap its content in aliases >>> (which might look unfortunate, because boards end doing it, in different >>> ways). >> >> I think this is all theoretical at the moment since we don't actually >> model the flash functions of this board (at least I haven't tested >> that at all) and unless it somehow uses it in ways I'm unaware of I >> think currently only the ROM is used. >> >>> For this device we have: >>> >>> (qemu) info mtree >>> 0000000000000000-ffffffffffffffff (prio 0, i/o): system >>> 00000004fff00000-00000004ffffffff (prio 0, romd): sam460ex.flash >>> >>> I'm not familiar with this arch/machine, let's assume the system bus is >>> 32bit, and the flash has a 8bit word (we have 8 data lines connected to >>> the pflash). >> >> Maybe this can help: >> https://datasheet.octopart.com/PPC460EX-NUB800T-AMCC-datasheet-11553412.pdf >> http://www.acube-systems.biz/index.php?page=hardware&pid=5 >> >> Unfortunately I don't have any more detailed docs where it's explained >> more but according to the above and in my limited understanding the >> SoC could handle larger flash chips but this board has 512 MB. We have >> not changed it now because I'm not sure if it would break anything and >> I don't have time to test it so Marcus just added a comment to remind >> about this and we're happy with that for now and could come back to it >> separately. > > And that's good enough for what I'm trying to do in this series, namely > getting rid of unwarranted magic around pflash devices. > >>> The 'no image' is 1MiB. >>> >>> 1 MiB = 8 Mbit >>> 8 Mbit / 32 = 2 ^ 18 >>> We need 18 address lines to reach the whole flash. >>> >>> What happens if we connect a 2MiB flash? We need 19 addr lines. >>> >>> If we only have 18 lines to connect our flash, we can hardwire our last >>> line as 0 or 1. >>> >>> - line #17 hardwired as 0: >>> Only the bottom part of the flash is accessible (range 0x000000..0x0fffff). >>> CPU reading 0x4fff00000 read flash offset 0x0. >>> Using CFI it is still a announced as 2MiB. >>> >>> - line #17 hardwired as 1: >>> Only the top part of the flash is accessible (range 0x100000..0x1fffff). >>> Can we trigger any operation from the internal state machine (writing to >>> address 0x555, named @unlock_addr0 in QEMU) since all access are >>> hardwardwired on top of 1MiB...? >>> Yes we can, because the pflash only uses 11 bits for it's I/O, so all >>> writes are masked and hit the I/O internal unit. >>> CPU reading 0x4fff00000 read flash offset 0x100000 >>> >>> If we do have 19 lines dedicated to our chip and connect a 512KiB flash, >>> we 'll use 17 lines and let 2 lines unused. >>> Regardless the values on the lines #17 and #18, the flash will answer to >>> the value on lines #0..#16. This might be called MMIO aliasing, and is >>> what setup the @nb_mappings argument. >>> This example with nb_mappings=4 would mean: >>> "I have a 2MiB I/O space and a 512KiB flash, map it and create 3 aliases". > > Physical hardware does address lines. Hardwiring address lines leaves > part of the hardware unaddressable. Not decoding address lines gets the > same stuff mapped multiple times in the address space. Address lines is > also what makes sizes powers of two.
I'm amazed about how you sumarized... Thanks! > QEMU device models are software. Emulating address lines faithfully > there takes extra effort. A hack job that simply maps whatever size > wherever is easier. It's why we could do 7919 sectors of 5323 bytes for > a size of 42152837 bytes, and map it at address 0x12345678. > > Of course, none of our boards is that nuts. But this one exudes a bit > of a nutty flavor: with "-drive if=pflash,format=raw,file=1G.img", it > happily maps 1G at address 0x4fff00000. info mtree: > > address-space: memory > 0000000000000000-ffffffffffffffff (prio 0, i/o): system > 0000000000000000-000000001fffffff (prio 0, i/o): sdram-containers > 0000000000000000-000000001fffffff (prio 0, i/o): alias > ppc4xx.sdram0 @ppc4xx.sdram 0000000000000000-000000001fffffff > 0000000400000000-000000040003ffff (prio 0, ram): ppc440.l2cache_ram > 00000004bffd0000-00000004bffd00ff (prio 0, i/o): ohci > 00000004bffd0400-00000004bffd13ff (prio 0, i/o): ehci > 00000004bffd0400-00000004bffd040f (prio 0, i/o): capabilities > 00000004bffd0410-00000004bffd0453 (prio 0, i/o): operational > 00000004bffd0454-00000004bffd046b (prio 0, i/o): ports > 00000004ef600300-00000004ef600307 (prio 0, i/o): serial > 00000004ef600700-00000004ef600711 (prio 0, i/o): ppc4xx-i2c > 00000004ef600800-00000004ef600811 (prio 0, i/o): ppc4xx-i2c > ---> 00000004fff00000-000000053fefffff (prio 0, romd): sam460ex.flash > 0000000c08000000-0000000c0800ffff (prio 0, i/o): alias isa_mmio @io > 0000000000000000-000000000000ffff > 0000000c0ec00000-0000000c0ec800fe (prio 0, i/o): pci-container > 0000000c0ec00000-0000000c0ec00003 (prio 0, i/o): pci-conf-idx > 0000000c0ec00004-0000000c0ec00007 (prio 0, i/o): pci-conf-data > 0000000c0ec80000-0000000c0ec800fe (prio 0, i/o): pci.reg > > Looks like there's plenty of space before we crash into pci-container. > Still, boards emulating real hardware should not take such liberties. Good catch, this is a Chip Select slot and is limited to the max addressing the bus allow to this CS. >>> Back to the architecture, what matters here is that the CPU reset vector >>> is always user-controlled (mapped on a flash device). >>> This arch has reset_vector @0x4fffffffc. >>> You could also map a 256KiB pflash at 0x4fffc0000, as long as the reset >>> vector is covered. If you map it at 0x4fff00000 or 0x4fff80000 it won't! >>> >>> This explanation is not arch-specific (adapting the reset vector to each >>> arch). >>> >>>> >>>> I guess memory at the end of the address space remains unmapped when >>>> it's smaller than 1MiB. Again, useful outcomes seem unlikely. >>> >>> If you map a 512KiB flash at 0x4fff00000, then the reset vector is not >>> covered. At 0x4fff80000 it is. >> >> Yes, I said the same before but this would be a separate patch and >> would need more testing so it wasn't included in this series. Since it >> works as it is now this can wait until later when it can be cleaned >> up. If we want to model the actual board we don't have to consider >> different flash sizes than the 512 MB that the board has so everything >> else is probably "overthinking" it. > > Isn't modelling the actual board why we have the virtual board in the > first place? But I digress... > > [...] >>>> The physical hardware appears to have 512KiB of flash memory: >>>> https://eu.mouser.com/datasheet/2/268/atmel_AT49BV040B-1180330.pdf >>>> >>>> For now, just set the flash memory size to 1MiB regardless of image >>>> size, and document the mess. >>>> >>>> Cc: BALATON Zoltan <bala...@eik.bme.hu> >>>> Signed-off-by: Markus Armbruster <arm...@redhat.com> >>>> --- >>>> hw/ppc/sam460ex.c | 41 ++++++++++++++++++++++++++--------------- >>>> 1 file changed, 26 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c >>>> index 75250d49e4..0c919529f8 100644 >>>> --- a/hw/ppc/sam460ex.c >>>> +++ b/hw/ppc/sam460ex.c >>>> @@ -91,32 +91,43 @@ struct boot_info { >>>> >>>> static int sam460ex_load_uboot(void) >>>> { >>>> + /* >>>> + * 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 get >>>> + * mapped on top of its second half (0xFFF80000..0xFFFFFFFF), >>>> + * initialized from u-boot-sam460-20100605.bin. >>> >>> I think the correct check is: >>> >>> if (! something_mapped_at(0x4fffffffc)) { >>> rom_map("u-boot-sam460.bin", >>> 0x500000000 - sizeof("u-boot-sam460.bin")); >>> } >>> >>> Maybe: >>> >>> if (!memory_region_present(get_system_memory(), 0x4fffffffc)) { >>> /* Current uboot ROM is 512KiB */ >>> /* TODO check [0x500000000 - 512KiB,0x500000000 - 1] unmapped */ >>> rom_add_file_fixed(UBOOT_FILENAME, >>> UBOOT_LOAD_BASE | ((hwaddr)FLASH_BASE_H << 32), >>> -1); >>> } > > I think this should be addressed in separate patches. This one merely > kills bad magic around pflash, such as inheriting the flash's size from > the block backend without sanity checking. "... let him speak now or forever hold his peace."
