On 31/1/22 20:20, Niek Linnenbank wrote:
Hi Philippe,
On Mon, Jan 31, 2022 at 12:29 AM Philippe Mathieu-Daudé <f4...@amsat.org
<mailto:f4...@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.
Indeed they don't run on GitLab due to that flag, but I run them locally
before sending a SD/MMC pull request (along with older images that are
not available anymore on the internet but are still in my local Avocado
cache).
> 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.
Great.
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.
OK :/
Your use case is typically what I tried to solve with this model:
https://lore.kernel.org/qemu-devel/20210419094329.1402767-2-f4...@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.
OK, this is a good start point. I'll look at the memory accesses
(certainly not today, but that problem is of my interest).
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.
Regards,
Phil.
