Kernel/aarch64: Use MM APIs for MMIO #25103
Conversation
github-actions
bot
added
the
👀 pr-needs-review
spholz
force-pushed
the
aarch64-mmio-via-mm
branch
from
8cdd0f1
to
9902da9
Compare
| @@ -62,6 +62,7 @@ ErrorOr<FlatPtr> page_round_up(FlatPtr x) | |||
| // run. If we do, then Singleton would get re-initialized, causing | |||
| // the memory manager to be initialized twice! | |||
| static MemoryManager* s_the; | |||
| static SetOnce s_mm_initialized; | |||
There was a problem hiding this comment.
Nice use of a SetOnce here :)
Kernel/Arch/aarch64/RPi/AUX.h
Outdated
|
|
||
| struct AUXRegisters; | ||
|
|
||
| class AUX { |
There was a problem hiding this comment.
I'd highly suggest to not use a class here. Instead use a namespace and declare Memory::TypedMapping<AUXRegisters volatile> m_registers; in the cpp file so it's "private" to the implementation. Every other method can reside in the namespace which would make it less awkward without the():: call dereference each time.
There was a problem hiding this comment.
I just followed what other driver code in RPi/ does. But I could change it if you think this looks better without the class.
The nice thing about having a singleton class is that the MM region is automatically allocated on first use. But I guess I could also make the TypedMapping itself a singleton (that would be a bit weird tho)?
There was a problem hiding this comment.
A singleton of TypedMapping should be OK in my opinion. When you have more interesting things to add, make a structure and include it so that would be the singleton, in the cpp file (and not exported via a header).
The whole SOMETHING_SINGLETON::the() concept has proved to be a very clunky way to use global variables and made it so we generated (including me, and probably most of it) lots of ugly code over the years.
There was a problem hiding this comment.
I use a private struct for the singleton now, since constructing a TypedMapping requires the address as an argument. I tried using a custom InitFunction template parameter, but that won't work, since it expects function pointer returning a raw pointer.
We may actually need to revert to having a class for it once this code uses the devicetree, since the MiniUART code would then probably need to get the AUX via the devicetree node as a key. Or we require that the AUX driver is initialized first somehow.
spholz
force-pushed
the
aarch64-mmio-via-mm
branch
3 times, most recently
from
eda7710
to
30499de
Compare
spholz
force-pushed
the
aarch64-mmio-via-mm
branch
from
30499de
to
15ed8ee
Compare
The 'Memory::' prefixes looked out of place next to the other region allocation functions.
Most MMIO regions should not be cacheable, as MMIO accesses typically have side effects (and are therefore not idempotent).
The other allocations of the AHCI Command Table in this file already had that argument.
Framebuffers should never be cached.
This replaces all usages of Cacheable::Yes with MemoryType::Normal and
Cacheable::No with either MemoryType::NonCacheable or MemoryType::IO,
depending on the context.
The Page{Directory,Table}::set_cache_disabled function therefore also
has been replaced with a more appropriate set_memory_type_function.
Adding a memory_type "getter" would not be as easy, as some
architectures may not support all memory types, so getting the memory
type again may be a lossy conversion. The is_cache_disabled function
was never used, so just simply remove it altogether.
There is no difference between MemoryType::NonCacheable and
MemoryType::IO on x86 for now.
Other architectures currently don't respect the MemoryType at all.
This removes the old Region::set_write_combine function and replaces all usages of it with setting the MemoryType to NonCacheable instead.
We never used this attribute, so remove it. If we ever find a use for it, we should enable it with a new MemoryType 'WriteThrough' instead.
Yes, the page table entry format is very arch-specific. This FIXME was added during initial aarch64 bringup.
We don't support i686 anymore.
This causes serial output to stop working before MM is completely initialized, as MMIO is not usable before that.
spholz
force-pushed
the
aarch64-mmio-via-mm
branch
from
15ed8ee
to
8c216a9
Compare
This is yet another step towards better aarch64 devicetree and Pi 4 support.
Serial output will sadly stop working before MM is initialized, as we currently don't have a way to map/access MMIO memory that early with MM APIs.
AArch64 cares more than x86 about us actually setting the correct memory type, as it doesn't have something akin to x86 MTRRs or RISC-V PMAs.