On Mon, Jan 31, 2022 at 12:29 AM Philippe Mathieu-Daudé <
f4bug@amsat.org> wrote:
Hi Niek!
(+Mark FYI)
On 30/1/22 23:50, Niek Linnenbank wrote:
> Hi David,
>
> While I realize my response is quite late, I wanted to report this error
> I found when running the acceptance
> tests for the orangepi-pc machine using avocado:
Unfortunately I only run the full SD/MMC tests when I send a SD/MMC pull
request, so missed that here.
I understand. These tests are behind the AVOCADO_ALLOW_LARGE_STORAGE flag in avocado, so I guess they
don't run on gitlab as well, but I'm not sure about that.
> Basically the two tests freeze during the part where the U-Boot
> bootloader needs to detect the amount of memory. We model this in the
> hw/misc/allwinner-h3-dramc.c file.
> And when running the machine manually it shows an assert on
> 'alias->mapped_via_alias >= 0'. When running manually via gdb, I was
> able to collect this backtrace:
>
> $ gdb ./build/qemu-system-arm
> ...
> gdb) run -M orangepi-pc -nographic
> ./Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img
> ...
> U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> DRAM:
> qemu-system-arm: ../softmmu/memory.c:2588: memory_region_del_subregion:
> Assertion `alias->mapped_via_alias >= 0' failed.
...
> So it seems that the hw/misc/allwinner-h3-dramc.c file is using the call
> memory_region_set_address, where internally we are calling
> memory_region_del_subregion.
> The allwinner-h3-dramc.c file does use
> memory_region_add_subregion_overlap once in the realize function, but
> might use the memory_region_set_address multiple times.
> It looks to me this is the path where the assert comes in. If I revert
> this patch on current master, the machine boots without the assertion.
>
> Would you be able to help out how we can best resolve this? Ofcourse, if
> there is anything needed to be changed on the allwinner-h3-dramc.c file,
> I would be happy to prepare a patch for that.
David's patch LGTM and I think your model might be somehow abusing the
memory API, but I'd like to read on the DRAMCOM Control Register to
understand the allwinner_h3_dramc_map_rows() logic. I couldn't find a
reference looking at Allwinner_H3_Datasheet_V1.2.pdf.
I wonder if we could ignore implementing it.
Yes David's fix using memory_region_add_subregion_common inside memory_region_readd_subregion resolves the issue indeed.
Well the allwinner-h3-dramc.c module works OK for now, but it can certainly use improvements indeed.
And you're right, unfortunately the DRAMCOM device isn't documented in the datasheet as far as I know.
Your use case is typically what I tried to solve with this model:
https://lore.kernel.org/qemu-devel/20210419094329.1402767-2-f4bug@amsat.org/
In your case, @span_size is your amount of DRAM, and @region_size is the
area u-boot is scanning (and @offset is zero).
Could that work, or is DRAMCOM doing much more?
The current model in allwinner-h3-dramc.c is roughly based on the code that is
present in U-Boot in the file arm/arm/mach-sunxi/dram_sunxi_dw.c.
It
implements the low-level initialization of the memory controller, and
when running using Qemu the most important thing it needs to do is
detect the amount of memory. If it cannot accomplish this task, the U-Boot SPL won't boot properly or crash later. So what we have in
the
allwinner-h3-dramc.c implementation comes from the information and code
in the dram_sunxi_dw.c file in U-Boot, not the datasheet.
The proposal you send with span_size/region_size looks interesting indeed. It would be great if this could help
simplify the code in allwinner-h3-dramc.c. But it would require some effort to figure out if it can indeed replace the current
behavior.
Kind regards,
Niek
Thanks,
Phil.
P.D. reference to documentation welcome :)