Hi Anirudh, On 2026-07-19T18:03:31, Anirudh Srinivasan <[email protected]> wrote: > bios_emulator: Support IO port reads for Aspeed VGA cards > > The Aspeed VGA PCI device exposes an MMIO region in BAR2 that is mirrors > the same registers exposed by I/O ports. All the reads/writes done by > the VBIOS to I/O ports can be forwarded to this region. This allows > the probe to complete and we get video output in u-boot.
Please fix 'that is mirrors' and 'u-boot' (U-Boot). The 'probably not the right way' remark belongs below the '---' cut (Patch-notes if you are using patman), not in the committed message. > > This is probably not the right way to do this, and I'm open to feedback > on how this should be done. > > Signed-off-by: Anirudh Srinivasan <[email protected]> > > drivers/bios_emulator/atibios.c | 30 ++++++++++++++++++++++++++++++ > drivers/bios_emulator/besys.c | 12 ++++++++++++ > drivers/bios_emulator/include/biosemu.h | 5 +++++ > 3 files changed, 47 insertions(+) > diff --git a/drivers/bios_emulator/atibios.c b/drivers/bios_emulator/atibios.c > @@ -483,6 +485,34 @@ int biosemu_setup(struct udevice *pcidev, BE_VGAInfo > **vga_infop) > + /* > + * ASPEED VGA devices expose a relocatable I/O BAR (BAR2, 128 bytes) > + * that maps the legacy VGA ports 0x380-0x3ff. Use it to forward VGA > + * port I/O to the real card on platforms without x86-style I/O > + * port access. > + */ > + if (device_is_on_pci_bus(pcidev)) { > + u16 vendor; > + u32 bar; > + > + dm_pci_read_config16(pcidev, PCI_VENDOR_ID, &vendor); > + if (vendor == 0x1a03) { I don't think this is the right place. biosemu_setup() is the shared entry point for all bios_emulator users and shouldn't carry vendor-specific quirks. Please move this into the aspeed_vga driver and expose a small helper, e.g. void biosemu_set_vga_mmio(void *mmio); which the aspeed_vga probe calls after biosemu_setup() and before biosemu_run(). That keeps atibios.c generic and drops the vendor-ID check plus the extra includes. device_is_on_pci_bus() is also redundant - biosemu_setup() already calls dm_pci_get_bdf() unconditionally on pcidev a few lines above. If you keep the raw hex vendor, please use PCI_VENDOR_ID_ASPEED from include/pci_ids.h. > diff --git a/drivers/bios_emulator/atibios.c b/drivers/bios_emulator/atibios.c > @@ -483,6 +485,34 @@ int biosemu_setup(struct udevice *pcidev, BE_VGAInfo > **vga_infop) > + _BE_env.vga_mmio = \ > + dm_pci_io_to_virt( \ > + pcidev, > + bar & > PCI_BASE_ADDRESS_IO_MASK, > + 0x80, > + MAP_NOCACHE); The line-continuation backslashes are not needed inside parentheses and the indentation is odd. Please reformat as: _BE_env.vga_mmio = dm_pci_io_to_virt(pcidev, bar & PCI_BASE_ADDRESS_IO_MASK, 0x80, MAP_NOCACHE); Also 0x80 should be a named constant (the window size - matches the 0x380..0x3ff comment), and please check the error return: dm_pci_io_to_virt() can return NULL. > diff --git a/drivers/bios_emulator/atibios.c b/drivers/bios_emulator/atibios.c > @@ -483,6 +485,34 @@ int biosemu_setup(struct udevice *pcidev, BE_VGAInfo > **vga_infop) > + printf("videoboot: VGA I/O via IO BAR2 %p\n", > + _BE_env.vga_mmio); I suggest demoting to log_debug() - the existing 'videoboot:' printfs predate the log framework and we shouldn't add new ones. > diff --git a/drivers/bios_emulator/besys.c b/drivers/bios_emulator/besys.c > @@ -268,6 +268,12 @@ static u8 VGA_inpb (const int port) > { > u8 val = 0xff; > > + /* Forward to the real card via its memory-mapped VGA port window */ > + if (_BE_env.vga_mmio) { > + val = readb(_BE_env.vga_mmio + (port - 0x380)); > + return val; > + } > + The window base 0x380 appears here, in the outpb path and in atibios.c - please define it once (e.g. VGA_MMIO_BASE_PORT) alongside the vga_mmio field in biosemu.h. Also, since IS_VGA_PORT() now starts at 0x3b4 (patch 5), any port below 0x3b4 will never reach here, but the window physically covers 0x380..0x3ff - just to check, does the VBIOS ever touch 0x380..0x3b3, and if so should IS_VGA_PORT() be widened further? Worth a sentence in the commit message either way. > diff --git a/drivers/bios_emulator/besys.c b/drivers/bios_emulator/besys.c > @@ -358,6 +364,12 @@ to use. > static void VGA_outpb (int port, u8 val) > { > + /* Forward to the real card via its memory-mapped VGA port window */ > + if (_BE_env.vga_mmio) { > + writeb(val, _BE_env.vga_mmio + (port - 0x380)); > + return; > + } > + BE_inw()/BE_outw() decompose word accesses into two VGA_inpb/VGA_outpb calls, so 16-bit port ops produce two separate MMIO byte accesses. Probably fine for VGA index/data pairs, but please confirm the Aspeed BAR2 window tolerates that (some VGA-alike windows require a single 16-bit access to latch index+data atomically). Regards, Simon
