* [PATCH 0/3] Add emulation of AmigaOne XE board
@ 2023-10-05 22:12 BALATON Zoltan
2023-10-05 22:13 ` [PATCH 1/3] via-ide: Fix legacy mode emulation BALATON Zoltan
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-05 22:12 UTC (permalink / raw)
To: qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, philmd,
Bernhard Beschow, Rene Engel, vr_qemu
This small series adds amigaone PPC machine which can be emulated
mostly reusing existing models used by pegasos2 as these machines are
very similar. The reason to add another board is that AmigaOS has
different versions for different machines that only run on that
machine and the AmigaOne version is more common than the PegasosII one.
Also it's useful for debugging to be able to test with both machines
as problems in emulation can be identified if they occur on both
machines as opposed to problems specific only to one version. Since
this only needs a very minimal north bridge emulation and a simple
board code with everything else shared with pegasos2 it is not a big
maintenance burden.
The board uses a modofied U-Boot that is needed to boot AmigaOS which
is freely available and distributable under GPL (see comment in
hw/ppc/amigaone.c added in patch 3 for details on how to get firmware
binary) but the sources for it could not be recovered so I could not
reproduce it from source. Ideally this firmware should be included
here for convenience which should be OK due to its GPL licence but
since there's only a binary I did not include it here. Let me know if
that would be OK to include but even without that firmware adding the
machine is useful as users can get the rom binary separately which
they are used to as pegasos2 also needed a firmware image to boot
AmigaOS until recently.
I hope this could be merged for 8.2 with this series being the minimum
set of patches needed to add this machine. This was tested by a Few
people already and can run AmigaOS and Linux distro for AmigaOne well.
Regards,
BALATON Zoltan
BALATON Zoltan (3):
via-ide: Fix legacy mode emulation
hw/pci-host: Add emulation of Mai Logic Articia S
hw/ppc: Add emulation of AmigaOne XE board
MAINTAINERS | 8 +
configs/devices/ppc-softmmu/default.mak | 1 +
hw/ide/via.c | 35 +++-
hw/pci-host/Kconfig | 5 +
hw/pci-host/articia.c | 266 ++++++++++++++++++++++++
hw/pci-host/meson.build | 2 +
hw/ppc/Kconfig | 7 +
hw/ppc/amigaone.c | 164 +++++++++++++++
hw/ppc/meson.build | 2 +
include/hw/pci-host/articia.h | 17 ++
10 files changed, 502 insertions(+), 5 deletions(-)
create mode 100644 hw/pci-host/articia.c
create mode 100644 hw/ppc/amigaone.c
create mode 100644 include/hw/pci-host/articia.h
--
2.30.9
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-05 22:12 [PATCH 0/3] Add emulation of AmigaOne XE board BALATON Zoltan
@ 2023-10-05 22:13 ` BALATON Zoltan
2023-10-08 6:13 ` Mark Cave-Ayland
2023-10-05 22:13 ` [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S BALATON Zoltan
2023-10-05 22:13 ` [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board BALATON Zoltan
2 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-05 22:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, philmd,
Bernhard Beschow, Rene Engel, vr_qemu
The initial value for BARs were set in reset method for emulating
legacy mode at start but this does not work because PCI code resets
BARs after calling device reset method. Additionally the values
written to BARs were also wrong.
Move setting the BARs to a callback on writing the PCI config regsiter
that sets the compatibility mode (which firmwares needing this mode
seem to do) and fix their values to program it to use legacy port
numbers. As noted in a comment, we only do this when the BARs were
unset before, because logs from real machine show this is how real
chip works, even if it contradicts the data sheet which is not very
clear about this.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index fff23803a6..8186190207 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
PCI_STATUS_DEVSEL_MEDIUM);
- pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
- pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
- pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
- pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
- pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
/* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
@@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
pci_set_long(pci_conf + 0xc0, 0x00020001);
}
+static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
+ uint32_t val, int len)
+{
+ pci_default_write_config(pd, addr, val, len);
+ /*
+ * Only set BARs if they are unset. Logs from real hardware show that
+ * writing class_prog to enable compatibility mode after BARs were set
+ * (possibly by firmware) it will use ports set by BARs not ISA ports
+ * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
+ * But if 0x8a is written after reset without setting BARs then we want
+ * legacy ports (this is done by AmigaOne firmware for example).
+ */
+ if (addr == PCI_CLASS_PROG && val == 0x8a &&
+ pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
+ PCI_BASE_ADDRESS_SPACE_IO) {
+ pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
+ | PCI_BASE_ADDRESS_SPACE_IO);
+ pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
+ | PCI_BASE_ADDRESS_SPACE_IO);
+ pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
+ | PCI_BASE_ADDRESS_SPACE_IO);
+ pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
+ | PCI_BASE_ADDRESS_SPACE_IO);
+ /* BMIBA: 20-23h */
+ pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
+ | PCI_BASE_ADDRESS_SPACE_IO);
+ }
+}
+
static void via_ide_realize(PCIDevice *dev, Error **errp)
{
PCIIDEState *d = PCI_IDE(dev);
@@ -221,6 +245,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
/* Reason: only works as function of VIA southbridge */
dc->user_creatable = false;
+ k->config_write = via_ide_cfg_write;
k->realize = via_ide_realize;
k->exit = via_ide_exitfn;
k->vendor_id = PCI_VENDOR_ID_VIA;
--
2.30.9
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-05 22:12 [PATCH 0/3] Add emulation of AmigaOne XE board BALATON Zoltan
2023-10-05 22:13 ` [PATCH 1/3] via-ide: Fix legacy mode emulation BALATON Zoltan
@ 2023-10-05 22:13 ` BALATON Zoltan
2023-10-06 21:13 ` Volker Rümelin
2023-10-08 6:14 ` Mark Cave-Ayland
2023-10-05 22:13 ` [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board BALATON Zoltan
2 siblings, 2 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-05 22:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, philmd,
Bernhard Beschow, Rene Engel, vr_qemu
The Articia S is a generic chipset supporting several different CPUs
that were used on some PPC boards. This is a minimal emulation of the
parts needed for emulating the AmigaOne board.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/pci-host/Kconfig | 5 +
hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
hw/pci-host/meson.build | 2 +
include/hw/pci-host/articia.h | 17 +++
4 files changed, 290 insertions(+)
create mode 100644 hw/pci-host/articia.c
create mode 100644 include/hw/pci-host/articia.h
diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
index a07070eddf..33014c80a4 100644
--- a/hw/pci-host/Kconfig
+++ b/hw/pci-host/Kconfig
@@ -73,6 +73,11 @@ config SH_PCI
bool
select PCI
+config ARTICIA
+ bool
+ select PCI
+ select I8259
+
config MV64361
bool
select PCI
diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
new file mode 100644
index 0000000000..80558e1c47
--- /dev/null
+++ b/hw/pci-host/articia.c
@@ -0,0 +1,266 @@
+/*
+ * Mai Logic Articia S emulation
+ *
+ * Copyright (c) 2023 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "hw/pci/pci_device.h"
+#include "hw/pci/pci_host.h"
+#include "hw/irq.h"
+#include "hw/i2c/bitbang_i2c.h"
+#include "hw/intc/i8259.h"
+#include "hw/pci-host/articia.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
+
+OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
+struct ArticiaHostState {
+ PCIDevice parent_obj;
+
+ ArticiaState *as;
+};
+
+/* TYPE_ARTICIA */
+
+struct ArticiaState {
+ PCIHostState parent_obj;
+
+ qemu_irq irq[PCI_NUM_PINS];
+ MemoryRegion io;
+ MemoryRegion mem;
+ MemoryRegion reg;
+
+ bitbang_i2c_interface smbus;
+ uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
+ hwaddr gpio_base;
+ MemoryRegion gpio_reg;
+};
+
+static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
+{
+ ArticiaState *s = opaque;
+
+ return (s->gpio >> (addr * 8)) & 0xff;
+}
+
+static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned int size)
+{
+ ArticiaState *s = opaque;
+ uint32_t sh = addr * 8;
+
+ if (addr == 0) {
+ /* in bits read only? */
+ return;
+ }
+
+ if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
+ s->gpio &= ~(0xff << sh | 0xff);
+ s->gpio |= (val & 0xff) << sh;
+ s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
+ s->gpio & BIT(16) ?
+ !!(s->gpio & BIT(8)) : 1);
+ if ((s->gpio & BIT(17))) {
+ s->gpio &= ~BIT(0);
+ s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
+ !!(s->gpio & BIT(9)));
+ }
+ }
+}
+
+static const MemoryRegionOps articia_gpio_ops = {
+ .read = articia_gpio_read,
+ .write = articia_gpio_write,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 1,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
+{
+ ArticiaState *s = opaque;
+ uint64_t ret = UINT_MAX;
+
+ switch (addr) {
+ case 0xc00cf8:
+ ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
+ break;
+ case 0xe00cfc ... 0xe00cff:
+ ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
+ break;
+ case 0xf00000:
+ ret = pic_read_irq(isa_pic);
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
+ HWADDR_PRIx " %d\n", __func__, addr, size);
+ break;
+ }
+ return ret;
+}
+
+static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned int size)
+{
+ ArticiaState *s = opaque;
+
+ switch (addr) {
+ case 0xc00cf8:
+ pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
+ break;
+ case 0xe00cfc ... 0xe00cff:
+ pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
+ HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
+ break;
+ }
+}
+
+static const MemoryRegionOps articia_reg_ops = {
+ .read = articia_reg_read,
+ .write = articia_reg_write,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 4,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void articia_pcihost_set_irq(void *opaque, int n, int level)
+{
+ ArticiaState *s = opaque;
+ qemu_set_irq(s->irq[n], level);
+}
+
+static void articia_realize(DeviceState *dev, Error **errp)
+{
+ ArticiaState *s = ARTICIA(dev);
+ PCIHostState *h = PCI_HOST_BRIDGE(dev);
+ PCIDevice *pdev;
+
+ bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
+ memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
+ TYPE_ARTICIA, 4);
+
+ memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
+ memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
+ memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
+ TYPE_ARTICIA, 0x1000000);
+ memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
+
+ /* devfn_min is 8 that matches first PCI slot in AmigaOne */
+ h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
+ pci_swizzle_map_irq_fn, dev, &s->mem,
+ &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);
+ pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
+ TYPE_ARTICIA_PCI_HOST);
+ ARTICIA_PCI_HOST(pdev)->as = s;
+ pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
+
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
+ qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
+}
+
+static void articia_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = articia_realize;
+ set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+}
+
+/* TYPE_ARTICIA_PCI_HOST */
+
+static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
+ uint32_t val, int len)
+{
+ ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
+
+ pci_default_write_config(d, addr, val, len);
+ switch (addr) {
+ case 0x40:
+ s->gpio_base = val;
+ break;
+ case 0x44:
+ if (val != 0x11) {
+ /* FIXME what do the bits actually mean? */
+ break;
+ }
+ if (memory_region_is_mapped(&s->gpio_reg)) {
+ memory_region_del_subregion(&s->io, &s->gpio_reg);
+ }
+ memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
+ break;
+ }
+}
+
+static void articia_pci_host_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ k->config_write = articia_pci_host_cfg_write;
+ k->vendor_id = 0x10cc;
+ k->device_id = 0x0660;
+ k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge,
+ * not usable without the host-facing part
+ */
+ dc->user_creatable = false;
+}
+
+/* TYPE_ARTICIA_PCI_BRIDGE */
+
+static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ k->vendor_id = 0x10cc;
+ k->device_id = 0x0661;
+ k->class_id = PCI_CLASS_BRIDGE_HOST;
+ /*
+ * PCI-facing part of the host bridge,
+ * not usable without the host-facing part
+ */
+ dc->user_creatable = false;
+}
+
+static const TypeInfo articia_types[] = {
+ {
+ .name = TYPE_ARTICIA,
+ .parent = TYPE_PCI_HOST_BRIDGE,
+ .instance_size = sizeof(ArticiaState),
+ .class_init = articia_class_init,
+ },
+ {
+ .name = TYPE_ARTICIA_PCI_HOST,
+ .parent = TYPE_PCI_DEVICE,
+ .instance_size = sizeof(ArticiaHostState),
+ .class_init = articia_pci_host_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { },
+ },
+ },
+ {
+ .name = TYPE_ARTICIA_PCI_BRIDGE,
+ .parent = TYPE_PCI_DEVICE,
+ .instance_size = sizeof(PCIDevice),
+ .class_init = articia_pci_bridge_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { },
+ },
+ },
+};
+
+DEFINE_TYPES(articia_types)
diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
index 64eada76fe..40de48eb7f 100644
--- a/hw/pci-host/meson.build
+++ b/hw/pci-host/meson.build
@@ -20,6 +20,8 @@ pci_ss.add(when: 'CONFIG_GRACKLE_PCI', if_true: files('grackle.c'))
pci_ss.add(when: 'CONFIG_UNIN_PCI', if_true: files('uninorth.c'))
# PowerPC E500 boards
pci_ss.add(when: 'CONFIG_PPCE500_PCI', if_true: files('ppce500.c'))
+# AmigaOne
+pci_ss.add(when: 'CONFIG_ARTICIA', if_true: files('articia.c'))
# Pegasos2
pci_ss.add(when: 'CONFIG_MV64361', if_true: files('mv64361.c'))
diff --git a/include/hw/pci-host/articia.h b/include/hw/pci-host/articia.h
new file mode 100644
index 0000000000..529c240274
--- /dev/null
+++ b/include/hw/pci-host/articia.h
@@ -0,0 +1,17 @@
+/*
+ * Mai Logic Articia S emulation
+ *
+ * Copyright (c) 2023 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#ifndef ARTICIA_H
+#define ARTICIA_H
+
+#define TYPE_ARTICIA "articia"
+#define TYPE_ARTICIA_PCI_HOST "articia-pci-host"
+#define TYPE_ARTICIA_PCI_BRIDGE "articia-pci-bridge"
+
+#endif
--
2.30.9
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board
2023-10-05 22:12 [PATCH 0/3] Add emulation of AmigaOne XE board BALATON Zoltan
2023-10-05 22:13 ` [PATCH 1/3] via-ide: Fix legacy mode emulation BALATON Zoltan
2023-10-05 22:13 ` [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S BALATON Zoltan
@ 2023-10-05 22:13 ` BALATON Zoltan
2023-10-09 6:24 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-05 22:13 UTC (permalink / raw)
To: qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, philmd,
Bernhard Beschow, Rene Engel, vr_qemu
The AmigaOne is a rebranded MAI Teron board that uses U-Boot firmware
with patches to support AmigaOS and is very similar to pegasos2 so can
be easily emulated sharing most code with pegasos2. The reason to
emulate it is that AmigaOS comes in different versions for AmigaOne
and PegasosII which only have drivers for one machine and firmware so
these only run on the specific machine. Adding this board allows
another AmigaOS version to be used reusing already existing peagasos2
emulation. (The AmigaOne was the first of these boards so likely most
widespread which then inspired Pegasos that was later replaced with
PegasosII due to problems with Articia S, so these have a lot of
similarity. Pegasos mainly ran MorphOS while the PegasosII version of
AmigaOS was added later and therefore less common than the AmigaOne
version.)
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
MAINTAINERS | 8 ++
configs/devices/ppc-softmmu/default.mak | 1 +
hw/ppc/Kconfig | 7 +
hw/ppc/amigaone.c | 164 ++++++++++++++++++++++++
hw/ppc/meson.build | 2 +
5 files changed, 182 insertions(+)
create mode 100644 hw/ppc/amigaone.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f0e20fde6..03f908c153 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1490,6 +1490,14 @@ F: hw/pci-host/mv64361.c
F: hw/pci-host/mv643xx.h
F: include/hw/pci-host/mv64361.h
+amigaone
+M: BALATON Zoltan <balaton@eik.bme.hu>
+L: qemu-ppc@nongnu.org
+S: Maintained
+F: hw/ppc/amigaone.c
+F: hw/pci-host/articia.c
+F: include/hw/pci-host/articia.h
+
Virtual Open Firmware (VOF)
M: Alexey Kardashevskiy <aik@ozlabs.ru>
R: David Gibson <david@gibson.dropbear.id.au>
diff --git a/configs/devices/ppc-softmmu/default.mak b/configs/devices/ppc-softmmu/default.mak
index a887f5438b..b85fd2bcd7 100644
--- a/configs/devices/ppc-softmmu/default.mak
+++ b/configs/devices/ppc-softmmu/default.mak
@@ -14,6 +14,7 @@ CONFIG_SAM460EX=y
CONFIG_MAC_OLDWORLD=y
CONFIG_MAC_NEWWORLD=y
+CONFIG_AMIGAONE=y
CONFIG_PEGASOS2=y
# For PReP
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 5dfbf47ef5..56f0475a8e 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -69,6 +69,13 @@ config SAM460EX
select USB_OHCI
select FDT_PPC
+config AMIGAONE
+ bool
+ imply ATI_VGA
+ select ARTICIA
+ select VT82C686
+ select SMBUS_EEPROM
+
config PEGASOS2
bool
imply ATI_VGA
diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
new file mode 100644
index 0000000000..3589924c8a
--- /dev/null
+++ b/hw/ppc/amigaone.c
@@ -0,0 +1,164 @@
+/*
+ * QEMU Eyetech AmigaOne/Mai Logic Teron emulation
+ *
+ * Copyright (c) 2023 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qemu/datadir.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "hw/ppc/ppc.h"
+#include "hw/boards.h"
+#include "hw/loader.h"
+#include "hw/pci-host/articia.h"
+#include "hw/isa/vt82c686.h"
+#include "hw/ide/pci.h"
+#include "hw/i2c/smbus_eeprom.h"
+#include "hw/ppc/ppc.h"
+#include "sysemu/reset.h"
+#include "kvm_ppc.h"
+
+#define BUS_FREQ_HZ 100000000
+
+/*
+ * Firmware binary available at
+ * https://www.hyperion-entertainment.com/index.php/downloads?view=files&parent=28
+ * then "tail -c 524288 updater.image >u-boot-amigaone.bin"
+ *
+ * BIOS emulator in firmware cannot run QEMU vgabios and hangs on it, use
+ * -device VGA,romfile=VGABIOS-lgpl-latest.bin
+ * from http://www.nongnu.org/vgabios/ instead.
+ */
+#define PROM_FILENAME "u-boot-amigaone.bin"
+#define PROM_ADDR 0xfff00000
+#define PROM_SIZE (512 * KiB)
+
+static void amigaone_cpu_reset(void *opaque)
+{
+ PowerPCCPU *cpu = opaque;
+
+ cpu_reset(CPU(cpu));
+ cpu_ppc_tb_reset(&cpu->env);
+}
+
+static void fix_spd_data(uint8_t *spd)
+{
+ uint32_t bank_size = 4 * MiB * spd[31];
+ uint32_t rows = bank_size / spd[13] / spd[17];
+ spd[3] = ctz32(rows) - spd[4];
+}
+
+static void amigaone_init(MachineState *machine)
+{
+ PowerPCCPU *cpu;
+ CPUPPCState *env;
+ MemoryRegion *rom, *pci_mem, *mr;
+ const char *fwname = machine->firmware ?: PROM_FILENAME;
+ char *filename;
+ ssize_t sz;
+ PCIBus *pci_bus;
+ Object *via;
+ DeviceState *dev;
+ I2CBus *i2c_bus;
+ uint8_t *spd_data;
+ int i;
+
+ /* init CPU */
+ cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
+ env = &cpu->env;
+ if (PPC_INPUT(env) != PPC_FLAGS_INPUT_6xx) {
+ error_report("Incompatible CPU, only 6xx bus supported");
+ exit(1);
+ }
+ cpu_ppc_tb_init(env, BUS_FREQ_HZ / 4);
+ qemu_register_reset(amigaone_cpu_reset, cpu);
+
+ /* RAM */
+ if (machine->ram_size > 2 * GiB) {
+ error_report("RAM size more than 2 GiB is not supported");
+ exit(1);
+ }
+ memory_region_add_subregion(get_system_memory(), 0, machine->ram);
+ if (machine->ram_size < 1 * GiB + 32 * KiB) {
+ /* Firmware uses this area for startup */
+ mr = g_new(MemoryRegion, 1);
+ memory_region_init_ram(mr, NULL, "init-cache", 32 * KiB, &error_fatal);
+ memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
+ }
+
+ /* allocate and load firmware */
+ filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, fwname);
+ if (!filename) {
+ error_report("Could not find firmware '%s'", fwname);
+ exit(1);
+ }
+ rom = g_new(MemoryRegion, 1);
+ memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
+ memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
+ sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE);
+ if (sz <= 0 || sz > PROM_SIZE) {
+ error_report("Could not load firmware '%s'", filename);
+ exit(1);
+ }
+ g_free(filename);
+
+ /* Articia S */
+ dev = sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL);
+
+ i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
+ if (machine->ram_size > 512 * MiB) {
+ spd_data = spd_data_generate(SDR, machine->ram_size / 2);
+ } else {
+ spd_data = spd_data_generate(SDR, machine->ram_size);
+ }
+ fix_spd_data(spd_data);
+ smbus_eeprom_init_one(i2c_bus, 0x51, spd_data);
+ if (machine->ram_size > 512 * MiB) {
+ smbus_eeprom_init_one(i2c_bus, 0x52, spd_data);
+ }
+
+ pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
+ mr = g_new(MemoryRegion, 1);
+ memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
+ 0, 0x1000000);
+ memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
+ mr = g_new(MemoryRegion, 1);
+ memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
+ 0x80000000, 0x7d000000);
+ memory_region_add_subregion(get_system_memory(), 0x80000000, mr);
+ pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
+
+ /* VIA VT82c686B South Bridge (multifunction PCI device) */
+ via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
+ TYPE_VT82C686B_ISA));
+ object_property_add_alias(OBJECT(machine), "rtc-time",
+ object_resolve_path_component(via, "rtc"),
+ "date");
+ qdev_connect_gpio_out(DEVICE(via), 0,
+ qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
+ for (i = 0; i < PCI_NUM_PINS; i++) {
+ qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
+ "pirq", i));
+ }
+ pci_ide_create_devs(PCI_DEVICE(object_resolve_path_component(via, "ide")));
+ pci_vga_init(pci_bus);
+}
+
+static void amigaone_machine_init(MachineClass *mc)
+{
+ mc->desc = "Eyetech AmigaOne/Mai Logic Teron";
+ mc->init = amigaone_init;
+ mc->block_default_type = IF_IDE;
+ mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("7457_v1.2");
+ mc->default_display = "std";
+ mc->default_ram_id = "ram";
+ mc->default_ram_size = 128 * MiB;
+}
+
+DEFINE_MACHINE("amigaone", amigaone_machine_init)
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 7c2c52434a..0f76f4cce4 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -81,6 +81,8 @@ ppc_ss.add(when: 'CONFIG_E500', if_true: files(
))
# PowerPC 440 Xilinx ML507 reference board.
ppc_ss.add(when: 'CONFIG_VIRTEX', if_true: files('virtex_ml507.c'))
+# a1xe
+ppc_ss.add(when: 'CONFIG_AMIGAONE', if_true: files('amigaone.c'))
# Pegasos2
ppc_ss.add(when: 'CONFIG_PEGASOS2', if_true: files('pegasos2.c'))
--
2.30.9
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-05 22:13 ` [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S BALATON Zoltan
@ 2023-10-06 21:13 ` Volker Rümelin
2023-10-07 10:21 ` BALATON Zoltan
2023-10-08 6:14 ` Mark Cave-Ayland
1 sibling, 1 reply; 24+ messages in thread
From: Volker Rümelin @ 2023-10-06 21:13 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, philmd,
Bernhard Beschow, Rene Engel
Am 06.10.23 um 00:13 schrieb BALATON Zoltan:
> The Articia S is a generic chipset supporting several different CPUs
> that were used on some PPC boards. This is a minimal emulation of the
> parts needed for emulating the AmigaOne board.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/pci-host/Kconfig | 5 +
> hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
> hw/pci-host/meson.build | 2 +
> include/hw/pci-host/articia.h | 17 +++
> 4 files changed, 290 insertions(+)
> create mode 100644 hw/pci-host/articia.c
> create mode 100644 include/hw/pci-host/articia.h
> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
> new file mode 100644
> index 0000000000..80558e1c47
> --- /dev/null
> +++ b/hw/pci-host/articia.c
> @@ -0,0 +1,266 @@
> +/*
> + * Mai Logic Articia S emulation
> + *
> + * Copyright (c) 2023 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "hw/pci/pci_device.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/irq.h"
> +#include "hw/i2c/bitbang_i2c.h"
> +#include "hw/intc/i8259.h"
> +#include "hw/pci-host/articia.h"
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
> +struct ArticiaHostState {
> + PCIDevice parent_obj;
> +
> + ArticiaState *as;
> +};
> +
> +/* TYPE_ARTICIA */
> +
> +struct ArticiaState {
> + PCIHostState parent_obj;
> +
> + qemu_irq irq[PCI_NUM_PINS];
> + MemoryRegion io;
> + MemoryRegion mem;
> + MemoryRegion reg;
> +
> + bitbang_i2c_interface smbus;
> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
> + hwaddr gpio_base;
> + MemoryRegion gpio_reg;
> +};
> +
> +static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> + ArticiaState *s = opaque;
> +
> + return (s->gpio >> (addr * 8)) & 0xff;
> +}
> +
> +static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned int size)
> +{
> + ArticiaState *s = opaque;
> + uint32_t sh = addr * 8;
> +
> + if (addr == 0) {
> + /* in bits read only? */
> + return;
> + }
> +
> + if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
> + s->gpio &= ~(0xff << sh | 0xff);
> + s->gpio |= (val & 0xff) << sh;
> + s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
> + s->gpio & BIT(16) ?
> + !!(s->gpio & BIT(8)) : 1);
> + if ((s->gpio & BIT(17))) {
> + s->gpio &= ~BIT(0);
> + s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
> + !!(s->gpio & BIT(9)));
> + }
> + }
> +}
> +
> +static const MemoryRegionOps articia_gpio_ops = {
> + .read = articia_gpio_read,
> + .write = articia_gpio_write,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 1,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> + ArticiaState *s = opaque;
> + uint64_t ret = UINT_MAX;
> +
> + switch (addr) {
> + case 0xc00cf8:
> + ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
> + break;
> + case 0xe00cfc ... 0xe00cff:
> + ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
> + break;
> + case 0xf00000:
> + ret = pic_read_irq(isa_pic);
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
> + HWADDR_PRIx " %d\n", __func__, addr, size);
> + break;
> + }
> + return ret;
> +}
> +
> +static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned int size)
> +{
> + ArticiaState *s = opaque;
> +
> + switch (addr) {
> + case 0xc00cf8:
> + pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
> + break;
> + case 0xe00cfc ... 0xe00cff:
> + pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
> + HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps articia_reg_ops = {
> + .read = articia_reg_read,
> + .write = articia_reg_write,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 4,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void articia_pcihost_set_irq(void *opaque, int n, int level)
> +{
> + ArticiaState *s = opaque;
> + qemu_set_irq(s->irq[n], level);
> +}
> +
> +static void articia_realize(DeviceState *dev, Error **errp)
> +{
> + ArticiaState *s = ARTICIA(dev);
> + PCIHostState *h = PCI_HOST_BRIDGE(dev);
> + PCIDevice *pdev;
> +
> + bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
> + memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
> + TYPE_ARTICIA, 4);
> +
> + memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
> + memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
> + memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
> + TYPE_ARTICIA, 0x1000000);
> + memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
> +
> + /* devfn_min is 8 that matches first PCI slot in AmigaOne */
> + h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
> + pci_swizzle_map_irq_fn, dev, &s->mem,
> + &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);
Hi,
pci_swizzle_map_irq_fn() doesn't correctly map all AmigaOne PCI
interrupt pin on device to interrupt pin on connector connections.
I found this mapping and I think it's correct. For the VIA Southbridge
at 00:07.0 it's the only possible correct mapping. All PCI functions
interrupts are connected internally.
diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
index d9f052e3f0..2ad011af9c 100644
--- a/hw/pci-host/articia.c
+++ b/hw/pci-host/articia.c
@@ -138,6 +138,29 @@ static void articia_pcihost_set_irq(void *opaque,
int n, int level)
qemu_set_irq(s->irq[n], level);
}
+/*
+ * AmigaOne SE PCI slot to IRQ routing
+ *
+ * repository: https://source.denx.de/u-boot/custodians/u-boot-avr32.git
+ * refspec: v2010.06
+ * file: board/MAI/AmigaOneG3SE/articiaS_pci.c
+ */
+static int amigaone_pcihost_bus0_map_irq(PCIDevice *pdev, int pin)
+{
+ switch(PCI_SLOT(pdev->devfn))
+ {
+ case 6: /* On board ethernet */
+ return 3;
+ case 7: /* Southbridge */
+ return pin;
+ default:
+ break;
+ }
+
+ /* Slot 1 Device 8, Slot 2 Device 9, Slot 3 Device 10 */
+ return pci_swizzle(PCI_SLOT(pdev->devfn), pin);
+}
+
static void articia_realize(DeviceState *dev, Error **errp)
{
ArticiaState *s = ARTICIA(dev);
@@ -155,7 +178,7 @@ static void articia_realize(DeviceState *dev, Error
**errp)
memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
- pci_swizzle_map_irq_fn, dev,
+ amigaone_pcihost_bus0_map_irq, dev,
&s->mem, &s->io, 0, 4, TYPE_PCI_BUS);
pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
TYPE_ARTICIA_PCI_HOST);
With best regards,
Volker
> + pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
> + TYPE_ARTICIA_PCI_HOST);
> + ARTICIA_PCI_HOST(pdev)->as = s;
> + pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
> +
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
> + qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
> +}
> +
> +static void articia_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = articia_realize;
> + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +}
> +
> +/* TYPE_ARTICIA_PCI_HOST */
> +
> +static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
> + uint32_t val, int len)
> +{
> + ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
> +
> + pci_default_write_config(d, addr, val, len);
> + switch (addr) {
> + case 0x40:
> + s->gpio_base = val;
> + break;
> + case 0x44:
> + if (val != 0x11) {
> + /* FIXME what do the bits actually mean? */
> + break;
> + }
> + if (memory_region_is_mapped(&s->gpio_reg)) {
> + memory_region_del_subregion(&s->io, &s->gpio_reg);
> + }
> + memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
> + break;
> + }
> +}
> +
> +static void articia_pci_host_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> + k->config_write = articia_pci_host_cfg_write;
> + k->vendor_id = 0x10cc;
> + k->device_id = 0x0660;
> + k->class_id = PCI_CLASS_BRIDGE_HOST;
> + /*
> + * PCI-facing part of the host bridge,
> + * not usable without the host-facing part
> + */
> + dc->user_creatable = false;
> +}
> +
> +/* TYPE_ARTICIA_PCI_BRIDGE */
> +
> +static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> + k->vendor_id = 0x10cc;
> + k->device_id = 0x0661;
> + k->class_id = PCI_CLASS_BRIDGE_HOST;
> + /*
> + * PCI-facing part of the host bridge,
> + * not usable without the host-facing part
> + */
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo articia_types[] = {
> + {
> + .name = TYPE_ARTICIA,
> + .parent = TYPE_PCI_HOST_BRIDGE,
> + .instance_size = sizeof(ArticiaState),
> + .class_init = articia_class_init,
> + },
> + {
> + .name = TYPE_ARTICIA_PCI_HOST,
> + .parent = TYPE_PCI_DEVICE,
> + .instance_size = sizeof(ArticiaHostState),
> + .class_init = articia_pci_host_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> + { },
> + },
> + },
> + {
> + .name = TYPE_ARTICIA_PCI_BRIDGE,
> + .parent = TYPE_PCI_DEVICE,
> + .instance_size = sizeof(PCIDevice),
> + .class_init = articia_pci_bridge_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> + { },
> + },
> + },
> +};
> +
> +DEFINE_TYPES(articia_types)
>
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-06 21:13 ` Volker Rümelin
@ 2023-10-07 10:21 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-07 10:21 UTC (permalink / raw)
To: Volker Rümelin
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel
[-- Attachment #1: Type: text/plain, Size: 12143 bytes --]
On Fri, 6 Oct 2023, Volker Rümelin wrote:
> Am 06.10.23 um 00:13 schrieb BALATON Zoltan:
>> The Articia S is a generic chipset supporting several different CPUs
>> that were used on some PPC boards. This is a minimal emulation of the
>> parts needed for emulating the AmigaOne board.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/pci-host/Kconfig | 5 +
>> hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
>> hw/pci-host/meson.build | 2 +
>> include/hw/pci-host/articia.h | 17 +++
>> 4 files changed, 290 insertions(+)
>> create mode 100644 hw/pci-host/articia.c
>> create mode 100644 include/hw/pci-host/articia.h
>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>> new file mode 100644
>> index 0000000000..80558e1c47
>> --- /dev/null
>> +++ b/hw/pci-host/articia.c
>> @@ -0,0 +1,266 @@
>> +/*
>> + * Mai Logic Articia S emulation
>> + *
>> + * Copyright (c) 2023 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qapi/error.h"
>> +#include "hw/pci/pci_device.h"
>> +#include "hw/pci/pci_host.h"
>> +#include "hw/irq.h"
>> +#include "hw/i2c/bitbang_i2c.h"
>> +#include "hw/intc/i8259.h"
>> +#include "hw/pci-host/articia.h"
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>> +struct ArticiaHostState {
>> + PCIDevice parent_obj;
>> +
>> + ArticiaState *as;
>> +};
>> +
>> +/* TYPE_ARTICIA */
>> +
>> +struct ArticiaState {
>> + PCIHostState parent_obj;
>> +
>> + qemu_irq irq[PCI_NUM_PINS];
>> + MemoryRegion io;
>> + MemoryRegion mem;
>> + MemoryRegion reg;
>> +
>> + bitbang_i2c_interface smbus;
>> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
>> + hwaddr gpio_base;
>> + MemoryRegion gpio_reg;
>> +};
>> +
>> +static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> + ArticiaState *s = opaque;
>> +
>> + return (s->gpio >> (addr * 8)) & 0xff;
>> +}
>> +
>> +static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
>> + unsigned int size)
>> +{
>> + ArticiaState *s = opaque;
>> + uint32_t sh = addr * 8;
>> +
>> + if (addr == 0) {
>> + /* in bits read only? */
>> + return;
>> + }
>> +
>> + if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
>> + s->gpio &= ~(0xff << sh | 0xff);
>> + s->gpio |= (val & 0xff) << sh;
>> + s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
>> + s->gpio & BIT(16) ?
>> + !!(s->gpio & BIT(8)) : 1);
>> + if ((s->gpio & BIT(17))) {
>> + s->gpio &= ~BIT(0);
>> + s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
>> + !!(s->gpio & BIT(9)));
>> + }
>> + }
>> +}
>> +
>> +static const MemoryRegionOps articia_gpio_ops = {
>> + .read = articia_gpio_read,
>> + .write = articia_gpio_write,
>> + .valid.min_access_size = 1,
>> + .valid.max_access_size = 1,
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> + ArticiaState *s = opaque;
>> + uint64_t ret = UINT_MAX;
>> +
>> + switch (addr) {
>> + case 0xc00cf8:
>> + ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
>> + break;
>> + case 0xe00cfc ... 0xe00cff:
>> + ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
>> + break;
>> + case 0xf00000:
>> + ret = pic_read_irq(isa_pic);
>> + break;
>> + default:
>> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
>> + HWADDR_PRIx " %d\n", __func__, addr, size);
>> + break;
>> + }
>> + return ret;
>> +}
>> +
>> +static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
>> + unsigned int size)
>> +{
>> + ArticiaState *s = opaque;
>> +
>> + switch (addr) {
>> + case 0xc00cf8:
>> + pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
>> + break;
>> + case 0xe00cfc ... 0xe00cff:
>> + pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
>> + break;
>> + default:
>> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
>> + HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
>> + break;
>> + }
>> +}
>> +
>> +static const MemoryRegionOps articia_reg_ops = {
>> + .read = articia_reg_read,
>> + .write = articia_reg_write,
>> + .valid.min_access_size = 1,
>> + .valid.max_access_size = 4,
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static void articia_pcihost_set_irq(void *opaque, int n, int level)
>> +{
>> + ArticiaState *s = opaque;
>> + qemu_set_irq(s->irq[n], level);
>> +}
>> +
>> +static void articia_realize(DeviceState *dev, Error **errp)
>> +{
>> + ArticiaState *s = ARTICIA(dev);
>> + PCIHostState *h = PCI_HOST_BRIDGE(dev);
>> + PCIDevice *pdev;
>> +
>> + bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>> + memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
>> + TYPE_ARTICIA, 4);
>> +
>> + memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
>> + memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
>> + memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>> + TYPE_ARTICIA, 0x1000000);
>> + memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>> +
>> + /* devfn_min is 8 that matches first PCI slot in AmigaOne */
>> + h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
>> + pci_swizzle_map_irq_fn, dev, &s->mem,
>> + &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);
>
> Hi,
>
> pci_swizzle_map_irq_fn() doesn't correctly map all AmigaOne PCI
> interrupt pin on device to interrupt pin on connector connections.
>
> I found this mapping and I think it's correct. For the VIA Southbridge
> at 00:07.0 it's the only possible correct mapping. All PCI functions
> interrupts are connected internally.
I think you're right, thanks a lot for finding this, I'll include this
change in v2 but wait a bit more with that if review comes up with
something else or R-b tags I can add then.
Regards,
BALATON Zoltan
> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
> index d9f052e3f0..2ad011af9c 100644
> --- a/hw/pci-host/articia.c
> +++ b/hw/pci-host/articia.c
> @@ -138,6 +138,29 @@ static void articia_pcihost_set_irq(void *opaque,
> int n, int level)
> qemu_set_irq(s->irq[n], level);
> }
>
> +/*
> + * AmigaOne SE PCI slot to IRQ routing
> + *
> + * repository: https://source.denx.de/u-boot/custodians/u-boot-avr32.git
> + * refspec: v2010.06
> + * file: board/MAI/AmigaOneG3SE/articiaS_pci.c
> + */
> +static int amigaone_pcihost_bus0_map_irq(PCIDevice *pdev, int pin)
> +{
> + switch(PCI_SLOT(pdev->devfn))
> + {
> + case 6: /* On board ethernet */
> + return 3;
> + case 7: /* Southbridge */
> + return pin;
> + default:
> + break;
> + }
> +
> + /* Slot 1 Device 8, Slot 2 Device 9, Slot 3 Device 10 */
> + return pci_swizzle(PCI_SLOT(pdev->devfn), pin);
> +}
> +
> static void articia_realize(DeviceState *dev, Error **errp)
> {
> ArticiaState *s = ARTICIA(dev);
> @@ -155,7 +178,7 @@ static void articia_realize(DeviceState *dev, Error
> **errp)
> memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>
> h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
> - pci_swizzle_map_irq_fn, dev,
> + amigaone_pcihost_bus0_map_irq, dev,
> &s->mem, &s->io, 0, 4, TYPE_PCI_BUS);
> pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
> TYPE_ARTICIA_PCI_HOST);
>
> With best regards,
> Volker
>
>
>> + pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
>> + TYPE_ARTICIA_PCI_HOST);
>> + ARTICIA_PCI_HOST(pdev)->as = s;
>> + pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
>> +
>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
>> + qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
>> +}
>> +
>> +static void articia_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->realize = articia_realize;
>> + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> +}
>> +
>> +/* TYPE_ARTICIA_PCI_HOST */
>> +
>> +static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
>> + uint32_t val, int len)
>> +{
>> + ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
>> +
>> + pci_default_write_config(d, addr, val, len);
>> + switch (addr) {
>> + case 0x40:
>> + s->gpio_base = val;
>> + break;
>> + case 0x44:
>> + if (val != 0x11) {
>> + /* FIXME what do the bits actually mean? */
>> + break;
>> + }
>> + if (memory_region_is_mapped(&s->gpio_reg)) {
>> + memory_region_del_subregion(&s->io, &s->gpio_reg);
>> + }
>> + memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
>> + break;
>> + }
>> +}
>> +
>> +static void articia_pci_host_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> + k->config_write = articia_pci_host_cfg_write;
>> + k->vendor_id = 0x10cc;
>> + k->device_id = 0x0660;
>> + k->class_id = PCI_CLASS_BRIDGE_HOST;
>> + /*
>> + * PCI-facing part of the host bridge,
>> + * not usable without the host-facing part
>> + */
>> + dc->user_creatable = false;
>> +}
>> +
>> +/* TYPE_ARTICIA_PCI_BRIDGE */
>> +
>> +static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> + k->vendor_id = 0x10cc;
>> + k->device_id = 0x0661;
>> + k->class_id = PCI_CLASS_BRIDGE_HOST;
>> + /*
>> + * PCI-facing part of the host bridge,
>> + * not usable without the host-facing part
>> + */
>> + dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo articia_types[] = {
>> + {
>> + .name = TYPE_ARTICIA,
>> + .parent = TYPE_PCI_HOST_BRIDGE,
>> + .instance_size = sizeof(ArticiaState),
>> + .class_init = articia_class_init,
>> + },
>> + {
>> + .name = TYPE_ARTICIA_PCI_HOST,
>> + .parent = TYPE_PCI_DEVICE,
>> + .instance_size = sizeof(ArticiaHostState),
>> + .class_init = articia_pci_host_class_init,
>> + .interfaces = (InterfaceInfo[]) {
>> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> + { },
>> + },
>> + },
>> + {
>> + .name = TYPE_ARTICIA_PCI_BRIDGE,
>> + .parent = TYPE_PCI_DEVICE,
>> + .instance_size = sizeof(PCIDevice),
>> + .class_init = articia_pci_bridge_class_init,
>> + .interfaces = (InterfaceInfo[]) {
>> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> + { },
>> + },
>> + },
>> +};
>> +
>> +DEFINE_TYPES(articia_types)
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-05 22:13 ` [PATCH 1/3] via-ide: Fix legacy mode emulation BALATON Zoltan
@ 2023-10-08 6:13 ` Mark Cave-Ayland
2023-10-08 11:08 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-10-08 6:13 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, philmd,
Bernhard Beschow, Rene Engel, vr_qemu
On 05/10/2023 23:13, BALATON Zoltan wrote:
> The initial value for BARs were set in reset method for emulating
> legacy mode at start but this does not work because PCI code resets
> BARs after calling device reset method.
This is certainly something I've noticed when testing previous versions of the VIA
patches. Perhaps it's worth a separate thread to the PCI devs?
> Additionally the values
> written to BARs were also wrong.
I don't believe this is correct: according to the datasheet the values on reset are
the ones given in the current reset code, so even if the reset function is overridden
at a later data during PCI bus reset, I would leave these for now since it is a
different issue.
> Move setting the BARs to a callback on writing the PCI config regsiter
> that sets the compatibility mode (which firmwares needing this mode
> seem to do) and fix their values to program it to use legacy port
> numbers. As noted in a comment, we only do this when the BARs were
> unset before, because logs from real machine show this is how real
> chip works, even if it contradicts the data sheet which is not very
> clear about this.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
> 1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..8186190207 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
> PCI_STATUS_DEVSEL_MEDIUM);
>
> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>
> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
> pci_set_long(pci_conf + 0xc0, 0x00020001);
> }
>
> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
> + uint32_t val, int len)
> +{
> + pci_default_write_config(pd, addr, val, len);
> + /*
> + * Only set BARs if they are unset. Logs from real hardware show that
> + * writing class_prog to enable compatibility mode after BARs were set
> + * (possibly by firmware) it will use ports set by BARs not ISA ports
> + * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
Can you remind me again where the references are to non-100% native mode? The only
thing I can find in Linux is
https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360
but that simply forces a switch to legacy mode, with no mention of "non-100% native
mode".
> + * But if 0x8a is written after reset without setting BARs then we want
> + * legacy ports (this is done by AmigaOne firmware for example).
> + */
What should happen here is that writing 0x8a should *always* switch to legacy mode,
so the BARs are unused...
> + if (addr == PCI_CLASS_PROG && val == 0x8a &&
> + pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
> + PCI_BASE_ADDRESS_SPACE_IO) {
> + pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
> + | PCI_BASE_ADDRESS_SPACE_IO);
> + pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
> + | PCI_BASE_ADDRESS_SPACE_IO);
> + pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
> + | PCI_BASE_ADDRESS_SPACE_IO);
> + pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
> + | PCI_BASE_ADDRESS_SPACE_IO);
> + /* BMIBA: 20-23h */
> + pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
> + | PCI_BASE_ADDRESS_SPACE_IO);
> + }
> +}
... but what you're doing here is effectively forcing the PCI BARs to the legacy
addresses. The reason we know this is because that is why you have the off-by-2 error
in BARs 1 and 3.
I could live with this approach for now if you're willing to adjust the comments
accordingly clarifying that forcing the PCI BARs to the legacy addresses is a hack to
be replaced in future with proper legacy ioports. At some point I will revive my PoC
series on top of Bernhard's last series that implements this properly.
> static void via_ide_realize(PCIDevice *dev, Error **errp)
> {
> PCIIDEState *d = PCI_IDE(dev);
> @@ -221,6 +245,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
> /* Reason: only works as function of VIA southbridge */
> dc->user_creatable = false;
>
> + k->config_write = via_ide_cfg_write;
> k->realize = via_ide_realize;
> k->exit = via_ide_exitfn;
> k->vendor_id = PCI_VENDOR_ID_VIA;
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-05 22:13 ` [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S BALATON Zoltan
2023-10-06 21:13 ` Volker Rümelin
@ 2023-10-08 6:14 ` Mark Cave-Ayland
2023-10-08 18:08 ` BALATON Zoltan
1 sibling, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-10-08 6:14 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, philmd,
Bernhard Beschow, Rene Engel, vr_qemu
On 05/10/2023 23:13, BALATON Zoltan wrote:
> The Articia S is a generic chipset supporting several different CPUs
> that were used on some PPC boards. This is a minimal emulation of the
> parts needed for emulating the AmigaOne board.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/pci-host/Kconfig | 5 +
> hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
> hw/pci-host/meson.build | 2 +
> include/hw/pci-host/articia.h | 17 +++
> 4 files changed, 290 insertions(+)
> create mode 100644 hw/pci-host/articia.c
> create mode 100644 include/hw/pci-host/articia.h
>
> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
> index a07070eddf..33014c80a4 100644
> --- a/hw/pci-host/Kconfig
> +++ b/hw/pci-host/Kconfig
> @@ -73,6 +73,11 @@ config SH_PCI
> bool
> select PCI
>
> +config ARTICIA
> + bool
> + select PCI
> + select I8259
> +
> config MV64361
> bool
> select PCI
> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
> new file mode 100644
> index 0000000000..80558e1c47
> --- /dev/null
> +++ b/hw/pci-host/articia.c
> @@ -0,0 +1,266 @@
> +/*
> + * Mai Logic Articia S emulation
> + *
> + * Copyright (c) 2023 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "hw/pci/pci_device.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/irq.h"
> +#include "hw/i2c/bitbang_i2c.h"
> +#include "hw/intc/i8259.h"
> +#include "hw/pci-host/articia.h"
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
> +struct ArticiaHostState {
> + PCIDevice parent_obj;
> +
> + ArticiaState *as;
> +};
> +
> +/* TYPE_ARTICIA */
> +
> +struct ArticiaState {
> + PCIHostState parent_obj;
> +
> + qemu_irq irq[PCI_NUM_PINS];
> + MemoryRegion io;
> + MemoryRegion mem;
> + MemoryRegion reg;
> +
> + bitbang_i2c_interface smbus;
> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
> + hwaddr gpio_base;
> + MemoryRegion gpio_reg;
> +};
These types above should be in the header file and not in the C file, as per our
current QOM guidelines.
> +static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> + ArticiaState *s = opaque;
> +
> + return (s->gpio >> (addr * 8)) & 0xff;
> +}
> +
> +static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned int size)
> +{
> + ArticiaState *s = opaque;
> + uint32_t sh = addr * 8;
> +
> + if (addr == 0) {
> + /* in bits read only? */
> + return;
> + }
> +
> + if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
> + s->gpio &= ~(0xff << sh | 0xff);
> + s->gpio |= (val & 0xff) << sh;
> + s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
> + s->gpio & BIT(16) ?
> + !!(s->gpio & BIT(8)) : 1);
> + if ((s->gpio & BIT(17))) {
> + s->gpio &= ~BIT(0);
> + s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
> + !!(s->gpio & BIT(9)));
> + }
> + }
> +}
> +
> +static const MemoryRegionOps articia_gpio_ops = {
> + .read = articia_gpio_read,
> + .write = articia_gpio_write,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 1,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> + ArticiaState *s = opaque;
> + uint64_t ret = UINT_MAX;
> +
> + switch (addr) {
> + case 0xc00cf8:
> + ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
> + break;
> + case 0xe00cfc ... 0xe00cff:
> + ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
> + break;
> + case 0xf00000:
> + ret = pic_read_irq(isa_pic);
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
> + HWADDR_PRIx " %d\n", __func__, addr, size);
> + break;
> + }
> + return ret;
> +}
> +
> +static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned int size)
> +{
> + ArticiaState *s = opaque;
> +
> + switch (addr) {
> + case 0xc00cf8:
> + pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
> + break;
> + case 0xe00cfc ... 0xe00cff:
> + pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
> + HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps articia_reg_ops = {
> + .read = articia_reg_read,
> + .write = articia_reg_write,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 4,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void articia_pcihost_set_irq(void *opaque, int n, int level)
> +{
> + ArticiaState *s = opaque;
> + qemu_set_irq(s->irq[n], level);
> +}
> +
> +static void articia_realize(DeviceState *dev, Error **errp)
> +{
> + ArticiaState *s = ARTICIA(dev);
> + PCIHostState *h = PCI_HOST_BRIDGE(dev);
> + PCIDevice *pdev;
> +
> + bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
> + memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
> + TYPE_ARTICIA, 4);
> +
> + memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
> + memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
> + memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
> + TYPE_ARTICIA, 0x1000000);
> + memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
> +
> + /* devfn_min is 8 that matches first PCI slot in AmigaOne */
> + h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
> + pci_swizzle_map_irq_fn, dev, &s->mem,
> + &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);
> + pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
> + TYPE_ARTICIA_PCI_HOST);
> + ARTICIA_PCI_HOST(pdev)->as = s;
> + pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
> +
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
> + qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
> +}
> +
> +static void articia_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = articia_realize;
> + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +}
> +
> +/* TYPE_ARTICIA_PCI_HOST */
> +
> +static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
> + uint32_t val, int len)
> +{
> + ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
> +
> + pci_default_write_config(d, addr, val, len);
> + switch (addr) {
> + case 0x40:
> + s->gpio_base = val;
> + break;
> + case 0x44:
> + if (val != 0x11) {
> + /* FIXME what do the bits actually mean? */
> + break;
> + }
> + if (memory_region_is_mapped(&s->gpio_reg)) {
> + memory_region_del_subregion(&s->io, &s->gpio_reg);
> + }
> + memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
> + break;
> + }
> +}
> +
> +static void articia_pci_host_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> + k->config_write = articia_pci_host_cfg_write;
> + k->vendor_id = 0x10cc;
> + k->device_id = 0x0660;
> + k->class_id = PCI_CLASS_BRIDGE_HOST;
> + /*
> + * PCI-facing part of the host bridge,
> + * not usable without the host-facing part
> + */
> + dc->user_creatable = false;
> +}
> +
> +/* TYPE_ARTICIA_PCI_BRIDGE */
> +
> +static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> + k->vendor_id = 0x10cc;
> + k->device_id = 0x0661;
> + k->class_id = PCI_CLASS_BRIDGE_HOST;
> + /*
> + * PCI-facing part of the host bridge,
> + * not usable without the host-facing part
> + */
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo articia_types[] = {
> + {
> + .name = TYPE_ARTICIA,
> + .parent = TYPE_PCI_HOST_BRIDGE,
> + .instance_size = sizeof(ArticiaState),
> + .class_init = articia_class_init,
> + },
> + {
> + .name = TYPE_ARTICIA_PCI_HOST,
> + .parent = TYPE_PCI_DEVICE,
> + .instance_size = sizeof(ArticiaHostState),
> + .class_init = articia_pci_host_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> + { },
> + },
> + },
> + {
> + .name = TYPE_ARTICIA_PCI_BRIDGE,
> + .parent = TYPE_PCI_DEVICE,
> + .instance_size = sizeof(PCIDevice),
> + .class_init = articia_pci_bridge_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> + { },
> + },
> + },
> +};
> +
> +DEFINE_TYPES(articia_types)
> diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
> index 64eada76fe..40de48eb7f 100644
> --- a/hw/pci-host/meson.build
> +++ b/hw/pci-host/meson.build
> @@ -20,6 +20,8 @@ pci_ss.add(when: 'CONFIG_GRACKLE_PCI', if_true: files('grackle.c'))
> pci_ss.add(when: 'CONFIG_UNIN_PCI', if_true: files('uninorth.c'))
> # PowerPC E500 boards
> pci_ss.add(when: 'CONFIG_PPCE500_PCI', if_true: files('ppce500.c'))
> +# AmigaOne
> +pci_ss.add(when: 'CONFIG_ARTICIA', if_true: files('articia.c'))
> # Pegasos2
> pci_ss.add(when: 'CONFIG_MV64361', if_true: files('mv64361.c'))
>
> diff --git a/include/hw/pci-host/articia.h b/include/hw/pci-host/articia.h
> new file mode 100644
> index 0000000000..529c240274
> --- /dev/null
> +++ b/include/hw/pci-host/articia.h
> @@ -0,0 +1,17 @@
> +/*
> + * Mai Logic Articia S emulation
> + *
> + * Copyright (c) 2023 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#ifndef ARTICIA_H
> +#define ARTICIA_H
> +
> +#define TYPE_ARTICIA "articia"
> +#define TYPE_ARTICIA_PCI_HOST "articia-pci-host"
> +#define TYPE_ARTICIA_PCI_BRIDGE "articia-pci-bridge"
> +
> +#endif
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-08 6:13 ` Mark Cave-Ayland
@ 2023-10-08 11:08 ` BALATON Zoltan
2023-10-09 9:06 ` Bernhard Beschow
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-08 11:08 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
> On 05/10/2023 23:13, BALATON Zoltan wrote:
>
>> The initial value for BARs were set in reset method for emulating
>> legacy mode at start but this does not work because PCI code resets
>> BARs after calling device reset method.
>
> This is certainly something I've noticed when testing previous versions of
> the VIA patches. Perhaps it's worth a separate thread to the PCI devs?
I think I brought up this back then but was told current PCI code won't
change and since that could break everything else that makes sense so this
is something that we should take as given and accomodate that.
>> Additionally the values
>> written to BARs were also wrong.
>
> I don't believe this is correct: according to the datasheet the values on
> reset are the ones given in the current reset code, so even if the reset
> function is overridden at a later data during PCI bus reset, I would leave
> these for now since it is a different issue.
Those values are missing the IO space bit for one so they can't be correct
as a BAR value no matter what the datasheet says. And since they are
ineffective now I think it's best to remove them to avoid confusion.
>> Move setting the BARs to a callback on writing the PCI config regsiter
>> that sets the compatibility mode (which firmwares needing this mode
>> seem to do) and fix their values to program it to use legacy port
>> numbers. As noted in a comment, we only do this when the BARs were
>> unset before, because logs from real machine show this is how real
>> chip works, even if it contradicts the data sheet which is not very
>> clear about this.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index fff23803a6..8186190207 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>> PCI_STATUS_DEVSEL_MEDIUM);
>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA:
>> 20-23h */
>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>> }
>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>> + uint32_t val, int len)
>> +{
>> + pci_default_write_config(pd, addr, val, len);
>> + /*
>> + * Only set BARs if they are unset. Logs from real hardware show that
>> + * writing class_prog to enable compatibility mode after BARs were set
>> + * (possibly by firmware) it will use ports set by BARs not ISA ports
>> + * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
>
> Can you remind me again where the references are to non-100% native mode? The
> only thing I can find in Linux is
> https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360
> but that simply forces a switch to legacy mode, with no mention of "non-100%
> native mode".
It was discussed somewhere in the via-ide thread we had when this was last
touched for pegasos2 in March 2020. Basically the non-100% native mode is
when ports are set by BARs but IRQs are still hard coded to 14-15. Linux
can work with all 3 possible modes: legacy (both ports and IRQs are hard
coded to ISA values), native (using BARs and PCI config 0x3c for a single
interrupt for both channels, vt82c686 data sheet does not document this
but vt8231 has a comment saying native mode only) and non-100% native mode
where BARs are effective to set port addresses but IRQs don't respect 0x3c
but use 14-15 as in legacy mode. Some machines only work in non-100%
native mode such as pegasos2 and Linux has some quirks for this. Other
OSes running on this machine work with what the firmware has set up and
can't work with anything else so we need to emulate what those OSes want
(which matches real hardware) because Linux can usually cope anyway. On
pegasso2 MorphOS uses BARs but expects IRQ 14-15 which is what the
firmware also sets up by setting mode to native in the PCI config of the
IDE func yet IRQs are fixed at 14-15. Linux forces its driver to use
legacy interrupts by setting mode back to legacy but it still uses BARs
and this is what it calls non-100% native mode. On amigaone firmware sets
legacy mode and AmigaOS does not change this but uses it with legacy ports
and IRQs. Linux finds the same and uses legacy mode on amigaone.
>> + * But if 0x8a is written after reset without setting BARs then we
>> want
>> + * legacy ports (this is done by AmigaOne firmware for example).
>> + */
>
> What should happen here is that writing 0x8a should *always* switch to legacy
> mode, so the BARs are unused...
Yes, but as we've found before that can't be emulated in QEMU due to ISA
emulation being static and only allows adding ports but not removing them
later so we can't switch between legacy ISA and PCI here so we use the
BARs for legacy ports as well to emulate that. The reason we only do this
if BARs are not yet set is because Linux changes this back to legacy mode
on pegasos2 but still uses BARs as shown in boot logs from real hardware
so it seems BARs take priority over legacy mode on real chip and Linux
only uses the mode reg to decide what IRQs to use. On amigaone firmware
writes 0x8a right after reset in which case we want legacy mode so this
works for both machines.
>> + if (addr == PCI_CLASS_PROG && val == 0x8a &&
>> + pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>> + PCI_BASE_ADDRESS_SPACE_IO) {
>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>> + | PCI_BASE_ADDRESS_SPACE_IO);
>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>> + | PCI_BASE_ADDRESS_SPACE_IO);
>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>> + | PCI_BASE_ADDRESS_SPACE_IO);
>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>> + | PCI_BASE_ADDRESS_SPACE_IO);
>> + /* BMIBA: 20-23h */
>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>> + | PCI_BASE_ADDRESS_SPACE_IO);
>> + }
>> +}
>
> ... but what you're doing here is effectively forcing the PCI BARs to the
> legacy addresses. The reason we know this is because that is why you have the
> off-by-2 error in BARs 1 and 3.
>
> I could live with this approach for now if you're willing to adjust the
> comments accordingly clarifying that forcing the PCI BARs to the legacy
> addresses is a hack to be replaced in future with proper legacy ioports. At
> some point I will revive my PoC series on top of Bernhard's last series that
> implements this properly.
That's fair enoough, I can try to clarify thid more in the comments and
commit message. I'll try to come up with something for v2.
Regards,
BALATON Zoltan
>> static void via_ide_realize(PCIDevice *dev, Error **errp)
>> {
>> PCIIDEState *d = PCI_IDE(dev);
>> @@ -221,6 +245,7 @@ static void via_ide_class_init(ObjectClass *klass, void
>> *data)
>> /* Reason: only works as function of VIA southbridge */
>> dc->user_creatable = false;
>> + k->config_write = via_ide_cfg_write;
>> k->realize = via_ide_realize;
>> k->exit = via_ide_exitfn;
>> k->vendor_id = PCI_VENDOR_ID_VIA;
>
>
> ATB,
>
> Mark.
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-08 6:14 ` Mark Cave-Ayland
@ 2023-10-08 18:08 ` BALATON Zoltan
2023-10-09 21:48 ` Mark Cave-Ayland
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-08 18:08 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
> On 05/10/2023 23:13, BALATON Zoltan wrote:
>
>> The Articia S is a generic chipset supporting several different CPUs
>> that were used on some PPC boards. This is a minimal emulation of the
>> parts needed for emulating the AmigaOne board.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/pci-host/Kconfig | 5 +
>> hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
>> hw/pci-host/meson.build | 2 +
>> include/hw/pci-host/articia.h | 17 +++
>> 4 files changed, 290 insertions(+)
>> create mode 100644 hw/pci-host/articia.c
>> create mode 100644 include/hw/pci-host/articia.h
>>
>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>> index a07070eddf..33014c80a4 100644
>> --- a/hw/pci-host/Kconfig
>> +++ b/hw/pci-host/Kconfig
>> @@ -73,6 +73,11 @@ config SH_PCI
>> bool
>> select PCI
>> +config ARTICIA
>> + bool
>> + select PCI
>> + select I8259
>> +
>> config MV64361
>> bool
>> select PCI
>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>> new file mode 100644
>> index 0000000000..80558e1c47
>> --- /dev/null
>> +++ b/hw/pci-host/articia.c
>> @@ -0,0 +1,266 @@
>> +/*
>> + * Mai Logic Articia S emulation
>> + *
>> + * Copyright (c) 2023 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qapi/error.h"
>> +#include "hw/pci/pci_device.h"
>> +#include "hw/pci/pci_host.h"
>> +#include "hw/irq.h"
>> +#include "hw/i2c/bitbang_i2c.h"
>> +#include "hw/intc/i8259.h"
>> +#include "hw/pci-host/articia.h"
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>> +struct ArticiaHostState {
>> + PCIDevice parent_obj;
>> +
>> + ArticiaState *as;
>> +};
>> +
>> +/* TYPE_ARTICIA */
>> +
>> +struct ArticiaState {
>> + PCIHostState parent_obj;
>> +
>> + qemu_irq irq[PCI_NUM_PINS];
>> + MemoryRegion io;
>> + MemoryRegion mem;
>> + MemoryRegion reg;
>> +
>> + bitbang_i2c_interface smbus;
>> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out)
>> */
>> + hwaddr gpio_base;
>> + MemoryRegion gpio_reg;
>> +};
>
> These types above should be in the header file and not in the C file, as per
> our current QOM guidelines.
I don't think there's such a guideline, at least I did not find any
mention of it in style and qom docs. It was necessary to move some type
declarations to headers for types that are embedded in other objects
because C needs the struct size for that, but I don't think that should be
a general thing when it's not needed.
The reason for that is that moving these to the header exposes internal
object structure to users that should not need to know that so it breaks
object encapsulation and also needs moving a bunch of includes to the
header which then makes the users of this type also include those headers
when they don't really need them but only need the type defines to
instantiate the object and that's all they should have access to. So I
think declaring types in the header should only be done for types that
aren't full devices and are meant to be embedded as part of another device
or a SoC but otherwise it's better to keep implementation closed and local
to the object and not expose it unless really needed, that's why these
are here.
If you insist I can move these but I don't think there's really such
recommendation and I don't think that's a good idea because of the above.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board
2023-10-05 22:13 ` [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board BALATON Zoltan
@ 2023-10-09 6:24 ` Philippe Mathieu-Daudé
2023-10-09 11:56 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-09 6:24 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: Nicholas Piggin, Daniel Henrique Barboza, clg, Bernhard Beschow,
Rene Engel, vr_qemu
Hi Zoltan,
On 6/10/23 00:13, BALATON Zoltan wrote:
> The AmigaOne is a rebranded MAI Teron board that uses U-Boot firmware
> with patches to support AmigaOS and is very similar to pegasos2 so can
> be easily emulated sharing most code with pegasos2. The reason to
> emulate it is that AmigaOS comes in different versions for AmigaOne
> and PegasosII which only have drivers for one machine and firmware so
> these only run on the specific machine. Adding this board allows
> another AmigaOS version to be used reusing already existing peagasos2
> emulation. (The AmigaOne was the first of these boards so likely most
> widespread which then inspired Pegasos that was later replaced with
> PegasosII due to problems with Articia S, so these have a lot of
> similarity. Pegasos mainly ran MorphOS while the PegasosII version of
> AmigaOS was added later and therefore less common than the AmigaOne
> version.)
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> MAINTAINERS | 8 ++
> configs/devices/ppc-softmmu/default.mak | 1 +
> hw/ppc/Kconfig | 7 +
> hw/ppc/amigaone.c | 164 ++++++++++++++++++++++++
> hw/ppc/meson.build | 2 +
> 5 files changed, 182 insertions(+)
> create mode 100644 hw/ppc/amigaone.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7f0e20fde6..03f908c153 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1490,6 +1490,14 @@ F: hw/pci-host/mv64361.c
> F: hw/pci-host/mv643xx.h
> F: include/hw/pci-host/mv64361.h
>
> +amigaone
'AmigaOne' like in subject and description?
> +M: BALATON Zoltan <balaton@eik.bme.hu>
> +L: qemu-ppc@nongnu.org
> +S: Maintained
> +F: hw/ppc/amigaone.c
> +F: hw/pci-host/articia.c
> +F: include/hw/pci-host/articia.h
> +
> Virtual Open Firmware (VOF)
> M: Alexey Kardashevskiy <aik@ozlabs.ru>
> +static void amigaone_init(MachineState *machine)
> +{
> + PowerPCCPU *cpu;
> + CPUPPCState *env;
> + MemoryRegion *rom, *pci_mem, *mr;
> + const char *fwname = machine->firmware ?: PROM_FILENAME;
> + char *filename;
> + ssize_t sz;
> + PCIBus *pci_bus;
> + Object *via;
> + DeviceState *dev;
> + I2CBus *i2c_bus;
> + uint8_t *spd_data;
> + int i;
> +
> + /* init CPU */
> + cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> + env = &cpu->env;
> + if (PPC_INPUT(env) != PPC_FLAGS_INPUT_6xx) {
> + error_report("Incompatible CPU, only 6xx bus supported");
> + exit(1);
> + }
> + cpu_ppc_tb_init(env, BUS_FREQ_HZ / 4);
> + qemu_register_reset(amigaone_cpu_reset, cpu);
> +
> + /* RAM */
> + if (machine->ram_size > 2 * GiB) {
> + error_report("RAM size more than 2 GiB is not supported");
> + exit(1);
> + }
> + memory_region_add_subregion(get_system_memory(), 0, machine->ram);
> + if (machine->ram_size < 1 * GiB + 32 * KiB) {
> + /* Firmware uses this area for startup */
This is odd. Does this machine really support 2GiB?
Could it be 1GiB max, mapped twice?
> + mr = g_new(MemoryRegion, 1);
> + memory_region_init_ram(mr, NULL, "init-cache", 32 * KiB, &error_fatal);
> + memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
> + }
> +
> + /* allocate and load firmware */
> + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, fwname);
> + if (!filename) {
> + error_report("Could not find firmware '%s'", fwname);
> + exit(1);
> + }
> + rom = g_new(MemoryRegion, 1);
> + memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
> + memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
> + sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE);
> + if (sz <= 0 || sz > PROM_SIZE) {
> + error_report("Could not load firmware '%s'", filename);
> + exit(1);
> + }
> + g_free(filename);
> +
> + /* Articia S */
> + dev = sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL);
> +
> + i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
> + if (machine->ram_size > 512 * MiB) {
> + spd_data = spd_data_generate(SDR, machine->ram_size / 2);
> + } else {
> + spd_data = spd_data_generate(SDR, machine->ram_size);
> + }
> + fix_spd_data(spd_data);
> + smbus_eeprom_init_one(i2c_bus, 0x51, spd_data);
> + if (machine->ram_size > 512 * MiB) {
> + smbus_eeprom_init_one(i2c_bus, 0x52, spd_data);
> + }
This seems to confirm my doubts, you use at most 2 SPD of 512MiB DIMMs,
so max for this machine is 1 GiB.
> + pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
> + mr = g_new(MemoryRegion, 1);
> + memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
> + 0, 0x1000000);
> + memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
> + mr = g_new(MemoryRegion, 1);
> + memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
> + 0x80000000, 0x7d000000);
> + memory_region_add_subregion(get_system_memory(), 0x80000000, mr);
> + pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
> +
> + /* VIA VT82c686B South Bridge (multifunction PCI device) */
> + via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
> + TYPE_VT82C686B_ISA));
> + object_property_add_alias(OBJECT(machine), "rtc-time",
> + object_resolve_path_component(via, "rtc"),
> + "date");
> + qdev_connect_gpio_out(DEVICE(via), 0,
> + qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
> + for (i = 0; i < PCI_NUM_PINS; i++) {
> + qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
> + "pirq", i));
> + }
> + pci_ide_create_devs(PCI_DEVICE(object_resolve_path_component(via, "ide")));
> + pci_vga_init(pci_bus);
> +}
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-08 11:08 ` BALATON Zoltan
@ 2023-10-09 9:06 ` Bernhard Beschow
2023-10-09 11:22 ` BALATON Zoltan
2023-10-09 10:03 ` Bernhard Beschow
2023-10-09 21:29 ` Mark Cave-Ayland
2 siblings, 1 reply; 24+ messages in thread
From: Bernhard Beschow @ 2023-10-09 9:06 UTC (permalink / raw)
To: BALATON Zoltan, Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Rene Engel, vr_qemu
Am 8. Oktober 2023 11:08:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>
>>> The initial value for BARs were set in reset method for emulating
>>> legacy mode at start but this does not work because PCI code resets
>>> BARs after calling device reset method.
>>
>> This is certainly something I've noticed when testing previous versions of the VIA patches. Perhaps it's worth a separate thread to the PCI devs?
>
>I think I brought up this back then but was told current PCI code won't change and since that could break everything else that makes sense so this is something that we should take as given and accomodate that.
Why not play safe like:
1. add a class property such as `reset_bar_addrs[PCI_NUM_REGIONS]`
2. set all elements to zero in `pci_device_class_init()`
3. respect `reset_bar_addrs` in `pci_reset_regions()`
4. assign the proper reset addresses of TYPE_VIA_IDE in `via_ide_class_init()`
That would pretty obviously preserve the behavior of existing device models while allowing TYPE_VIA_IDE to be reset properly. It would also perform the main part of the workaround in the code that exhibits the limitation, so the code could potentially be simplified at some point without impacting all PCI device models.
Best regards,
Bernhard
>
>>> Additionally the values
>>> written to BARs were also wrong.
>>
>> I don't believe this is correct: according to the datasheet the values on reset are the ones given in the current reset code, so even if the reset function is overridden at a later data during PCI bus reset, I would leave these for now since it is a different issue.
>
>Those values are missing the IO space bit for one so they can't be correct as a BAR value no matter what the datasheet says. And since they are ineffective now I think it's best to remove them to avoid confusion.
>
>>> Move setting the BARs to a callback on writing the PCI config regsiter
>>> that sets the compatibility mode (which firmwares needing this mode
>>> seem to do) and fix their values to program it to use legacy port
>>> numbers. As noted in a comment, we only do this when the BARs were
>>> unset before, because logs from real machine show this is how real
>>> chip works, even if it contradicts the data sheet which is not very
>>> clear about this.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..8186190207 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>> PCI_STATUS_DEVSEL_MEDIUM);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>>> }
>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>> + uint32_t val, int len)
>>> +{
>>> + pci_default_write_config(pd, addr, val, len);
>>> + /*
>>> + * Only set BARs if they are unset. Logs from real hardware show that
>>> + * writing class_prog to enable compatibility mode after BARs were set
>>> + * (possibly by firmware) it will use ports set by BARs not ISA ports
>>> + * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
>>
>> Can you remind me again where the references are to non-100% native mode? The only thing I can find in Linux is https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360 but that simply forces a switch to legacy mode, with no mention of "non-100% native mode".
>
>It was discussed somewhere in the via-ide thread we had when this was last touched for pegasos2 in March 2020. Basically the non-100% native mode is when ports are set by BARs but IRQs are still hard coded to 14-15. Linux can work with all 3 possible modes: legacy (both ports and IRQs are hard coded to ISA values), native (using BARs and PCI config 0x3c for a single interrupt for both channels, vt82c686 data sheet does not document this but vt8231 has a comment saying native mode only) and non-100% native mode where BARs are effective to set port addresses but IRQs don't respect 0x3c but use 14-15 as in legacy mode. Some machines only work in non-100% native mode such as pegasos2 and Linux has some quirks for this. Other OSes running on this machine work with what the firmware has set up and can't work with anything else so we need to emulate what those OSes want (which matches real hardware) because Linux can usually cope anyway. On pegasso2 MorphOS uses BARs but expects IRQ 14-15 which is what the firmware also sets up by setting mode to native in the PCI config of the IDE func yet IRQs are fixed at 14-15. Linux forces its driver to use legacy interrupts by setting mode back to legacy but it still uses BARs and this is what it calls non-100% native mode. On amigaone firmware sets legacy mode and AmigaOS does not change this but uses it with legacy ports and IRQs. Linux finds the same and uses legacy mode on amigaone.
>
>>> + * But if 0x8a is written after reset without setting BARs then we want
>>> + * legacy ports (this is done by AmigaOne firmware for example).
>>> + */
>>
>> What should happen here is that writing 0x8a should *always* switch to legacy mode, so the BARs are unused...
>
>Yes, but as we've found before that can't be emulated in QEMU due to ISA emulation being static and only allows adding ports but not removing them later so we can't switch between legacy ISA and PCI here so we use the BARs for legacy ports as well to emulate that. The reason we only do this if BARs are not yet set is because Linux changes this back to legacy mode on pegasos2 but still uses BARs as shown in boot logs from real hardware so it seems BARs take priority over legacy mode on real chip and Linux only uses the mode reg to decide what IRQs to use. On amigaone firmware writes 0x8a right after reset in which case we want legacy mode so this works for both machines.
>
>>> + if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>> + pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>> + PCI_BASE_ADDRESS_SPACE_IO) {
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + /* BMIBA: 20-23h */
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + }
>>> +}
>>
>> ... but what you're doing here is effectively forcing the PCI BARs to the legacy addresses. The reason we know this is because that is why you have the off-by-2 error in BARs 1 and 3.
>>
>> I could live with this approach for now if you're willing to adjust the comments accordingly clarifying that forcing the PCI BARs to the legacy addresses is a hack to be replaced in future with proper legacy ioports. At some point I will revive my PoC series on top of Bernhard's last series that implements this properly.
>
>That's fair enoough, I can try to clarify thid more in the comments and commit message. I'll try to come up with something for v2.
>
>Regards,
>BALATON Zoltan
>
>>> static void via_ide_realize(PCIDevice *dev, Error **errp)
>>> {
>>> PCIIDEState *d = PCI_IDE(dev);
>>> @@ -221,6 +245,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>>> /* Reason: only works as function of VIA southbridge */
>>> dc->user_creatable = false;
>>> + k->config_write = via_ide_cfg_write;
>>> k->realize = via_ide_realize;
>>> k->exit = via_ide_exitfn;
>>> k->vendor_id = PCI_VENDOR_ID_VIA;
>>
>>
>> ATB,
>>
>> Mark.
>>
>>
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-08 11:08 ` BALATON Zoltan
2023-10-09 9:06 ` Bernhard Beschow
@ 2023-10-09 10:03 ` Bernhard Beschow
2023-10-09 13:02 ` BALATON Zoltan
2023-10-09 21:29 ` Mark Cave-Ayland
2 siblings, 1 reply; 24+ messages in thread
From: Bernhard Beschow @ 2023-10-09 10:03 UTC (permalink / raw)
To: BALATON Zoltan, Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Rene Engel, vr_qemu
Am 8. Oktober 2023 11:08:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>
>>> The initial value for BARs were set in reset method for emulating
>>> legacy mode at start but this does not work because PCI code resets
>>> BARs after calling device reset method.
>>
>> This is certainly something I've noticed when testing previous versions of the VIA patches. Perhaps it's worth a separate thread to the PCI devs?
>
>I think I brought up this back then but was told current PCI code won't change and since that could break everything else that makes sense so this is something that we should take as given and accomodate that.
>
>>> Additionally the values
>>> written to BARs were also wrong.
>>
>> I don't believe this is correct: according to the datasheet the values on reset are the ones given in the current reset code, so even if the reset function is overridden at a later data during PCI bus reset, I would leave these for now since it is a different issue.
>
>Those values are missing the IO space bit for one so they can't be correct as a BAR value no matter what the datasheet says. And since they are ineffective now I think it's best to remove them to avoid confusion.
>
>>> Move setting the BARs to a callback on writing the PCI config regsiter
>>> that sets the compatibility mode (which firmwares needing this mode
>>> seem to do) and fix their values to program it to use legacy port
>>> numbers. As noted in a comment, we only do this when the BARs were
>>> unset before, because logs from real machine show this is how real
>>> chip works, even if it contradicts the data sheet which is not very
>>> clear about this.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..8186190207 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>> PCI_STATUS_DEVSEL_MEDIUM);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>>> }
>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>> + uint32_t val, int len)
>>> +{
>>> + pci_default_write_config(pd, addr, val, len);
>>> + /*
>>> + * Only set BARs if they are unset. Logs from real hardware show that
>>> + * writing class_prog to enable compatibility mode after BARs were set
>>> + * (possibly by firmware) it will use ports set by BARs not ISA ports
>>> + * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
>>
>> Can you remind me again where the references are to non-100% native mode? The only thing I can find in Linux is https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360 but that simply forces a switch to legacy mode, with no mention of "non-100% native mode".
>
>It was discussed somewhere in the via-ide thread we had when this was last touched for pegasos2 in March 2020. Basically the non-100% native mode is when ports are set by BARs but IRQs are still hard coded to 14-15. Linux can work with all 3 possible modes: legacy (both ports and IRQs are hard coded to ISA values), native (using BARs and PCI config 0x3c for a single interrupt for both channels, vt82c686 data sheet does not document this but vt8231 has a comment saying native mode only) and non-100% native mode where BARs are effective to set port addresses but IRQs don't respect 0x3c but use 14-15 as in legacy mode. Some machines only work in non-100% native mode such as pegasos2 and Linux has some quirks for this. Other OSes running on this machine work with what the firmware has set up and can't work with anything else so we need to emulate what those OSes want (which matches real hardware) because Linux can usually cope anyway. On pegasso2 MorphOS uses BARs but expects IRQ 14-15 which is what the firmware also sets up by setting mode to native in the PCI config of the IDE func yet IRQs are fixed at 14-15. Linux forces its driver to use legacy interrupts by setting mode back to legacy but it still uses BARs and this is what it calls non-100% native mode. On amigaone firmware sets legacy mode and AmigaOS does not change this but uses it with legacy ports and IRQs. Linux finds the same and uses legacy mode on amigaone.
>
>>> + * But if 0x8a is written after reset without setting BARs then we want
>>> + * legacy ports (this is done by AmigaOne firmware for example).
>>> + */
>>
>> What should happen here is that writing 0x8a should *always* switch to legacy mode, so the BARs are unused...
>
>Yes, but as we've found before that can't be emulated in QEMU due to ISA emulation being static and only allows adding ports but not removing them later so we can't switch between legacy ISA and PCI here so we use the BARs for legacy ports as well to emulate that. The reason we only do this if BARs are not yet set is because Linux changes this back to legacy mode on pegasos2 but still uses BARs
Just curious: How can you tell the difference in real hardware whether "raw" IO ports vs. BARs mapped to IO space are used?
> as shown in boot logs from real hardware
Could you provide links to such logs? That would be very helpful to have -- even in the code -- for documentation.
Best regards,
Bernhard
> so it seems BARs take priority over legacy mode on real chip and Linux only uses the mode reg to decide what IRQs to use. On amigaone firmware writes 0x8a right after reset in which case we want legacy mode so this works for both machines.
>
>>> + if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>> + pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>> + PCI_BASE_ADDRESS_SPACE_IO) {
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + /* BMIBA: 20-23h */
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + }
>>> +}
>>
>> ... but what you're doing here is effectively forcing the PCI BARs to the legacy addresses. The reason we know this is because that is why you have the off-by-2 error in BARs 1 and 3.
>>
>> I could live with this approach for now if you're willing to adjust the comments accordingly clarifying that forcing the PCI BARs to the legacy addresses is a hack to be replaced in future with proper legacy ioports. At some point I will revive my PoC series on top of Bernhard's last series that implements this properly.
>
>That's fair enoough, I can try to clarify thid more in the comments and commit message. I'll try to come up with something for v2.
>
>Regards,
>BALATON Zoltan
>
>>> static void via_ide_realize(PCIDevice *dev, Error **errp)
>>> {
>>> PCIIDEState *d = PCI_IDE(dev);
>>> @@ -221,6 +245,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>>> /* Reason: only works as function of VIA southbridge */
>>> dc->user_creatable = false;
>>> + k->config_write = via_ide_cfg_write;
>>> k->realize = via_ide_realize;
>>> k->exit = via_ide_exitfn;
>>> k->vendor_id = PCI_VENDOR_ID_VIA;
>>
>>
>> ATB,
>>
>> Mark.
>>
>>
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-09 9:06 ` Bernhard Beschow
@ 2023-10-09 11:22 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-09 11:22 UTC (permalink / raw)
To: Bernhard Beschow
Cc: Mark Cave-Ayland, qemu-devel, qemu-ppc, Nicholas Piggin,
Daniel Henrique Barboza, clg, philmd, Rene Engel, vr_qemu
On Mon, 9 Oct 2023, Bernhard Beschow wrote:
> Am 8. Oktober 2023 11:08:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>
>>>> The initial value for BARs were set in reset method for emulating
>>>> legacy mode at start but this does not work because PCI code resets
>>>> BARs after calling device reset method.
>>>
>>> This is certainly something I've noticed when testing previous versions of the VIA patches. Perhaps it's worth a separate thread to the PCI devs?
>>
>> I think I brought up this back then but was told current PCI code won't change and since that could break everything else that makes sense so this is something that we should take as given and accomodate that.
>
> Why not play safe like:
> 1. add a class property such as `reset_bar_addrs[PCI_NUM_REGIONS]`
> 2. set all elements to zero in `pci_device_class_init()`
> 3. respect `reset_bar_addrs` in `pci_reset_regions()`
> 4. assign the proper reset addresses of TYPE_VIA_IDE in `via_ide_class_init()`
>
> That would pretty obviously preserve the behavior of existing device
> models while allowing TYPE_VIA_IDE to be reset properly. It would also
> perform the main part of the workaround in the code that exhibits the
> limitation, so the code could potentially be simplified at some point
> without impacting all PCI device models.
That's a lot of complication for setting some values that will be
overwritten first thing after reset. Either the guest sets native mode and
writes the BARs itself so the reset values don't matter or it sets legacy
mode when the workaround has to set legacy ports (that strangely don't
match the reset values of the BARs) so sticking to those reset values
makes no sense to me as it does not help to run any guest just makes the
code more complex and harder to understand for no reason. So the patch
with clarified comment as Mark asked should do for now. I'll send that in
v2 later.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board
2023-10-09 6:24 ` Philippe Mathieu-Daudé
@ 2023-10-09 11:56 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-09 11:56 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, Bernhard Beschow, Rene Engel, vr_qemu
[-- Attachment #1: Type: text/plain, Size: 9402 bytes --]
On Mon, 9 Oct 2023, Philippe Mathieu-Daudé wrote:
> Hi Zoltan,
>
> On 6/10/23 00:13, BALATON Zoltan wrote:
>> The AmigaOne is a rebranded MAI Teron board that uses U-Boot firmware
>> with patches to support AmigaOS and is very similar to pegasos2 so can
>> be easily emulated sharing most code with pegasos2. The reason to
>> emulate it is that AmigaOS comes in different versions for AmigaOne
>> and PegasosII which only have drivers for one machine and firmware so
>> these only run on the specific machine. Adding this board allows
>> another AmigaOS version to be used reusing already existing peagasos2
>> emulation. (The AmigaOne was the first of these boards so likely most
>> widespread which then inspired Pegasos that was later replaced with
>> PegasosII due to problems with Articia S, so these have a lot of
>> similarity. Pegasos mainly ran MorphOS while the PegasosII version of
>> AmigaOS was added later and therefore less common than the AmigaOne
>> version.)
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> MAINTAINERS | 8 ++
>> configs/devices/ppc-softmmu/default.mak | 1 +
>> hw/ppc/Kconfig | 7 +
>> hw/ppc/amigaone.c | 164 ++++++++++++++++++++++++
>> hw/ppc/meson.build | 2 +
>> 5 files changed, 182 insertions(+)
>> create mode 100644 hw/ppc/amigaone.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7f0e20fde6..03f908c153 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1490,6 +1490,14 @@ F: hw/pci-host/mv64361.c
>> F: hw/pci-host/mv643xx.h
>> F: include/hw/pci-host/mv64361.h
>> +amigaone
>
> 'AmigaOne' like in subject and description?
The machine option is called amigaone and for pegasos2 and sam460ex I've
also used the machine name so for consistency it's the same here.
>> +M: BALATON Zoltan <balaton@eik.bme.hu>
>> +L: qemu-ppc@nongnu.org
>> +S: Maintained
>> +F: hw/ppc/amigaone.c
>> +F: hw/pci-host/articia.c
>> +F: include/hw/pci-host/articia.h
>> +
>> Virtual Open Firmware (VOF)
>> M: Alexey Kardashevskiy <aik@ozlabs.ru>
>
>
>> +static void amigaone_init(MachineState *machine)
>> +{
>> + PowerPCCPU *cpu;
>> + CPUPPCState *env;
>> + MemoryRegion *rom, *pci_mem, *mr;
>> + const char *fwname = machine->firmware ?: PROM_FILENAME;
>> + char *filename;
>> + ssize_t sz;
>> + PCIBus *pci_bus;
>> + Object *via;
>> + DeviceState *dev;
>> + I2CBus *i2c_bus;
>> + uint8_t *spd_data;
>> + int i;
>> +
>> + /* init CPU */
>> + cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>> + env = &cpu->env;
>> + if (PPC_INPUT(env) != PPC_FLAGS_INPUT_6xx) {
>> + error_report("Incompatible CPU, only 6xx bus supported");
>> + exit(1);
>> + }
>> + cpu_ppc_tb_init(env, BUS_FREQ_HZ / 4);
>> + qemu_register_reset(amigaone_cpu_reset, cpu);
>> +
>> + /* RAM */
>> + if (machine->ram_size > 2 * GiB) {
>> + error_report("RAM size more than 2 GiB is not supported");
>> + exit(1);
>> + }
>> + memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>> + if (machine->ram_size < 1 * GiB + 32 * KiB) {
>> + /* Firmware uses this area for startup */
>
> This is odd. Does this machine really support 2GiB?
>
> Could it be 1GiB max, mapped twice?
Apparently AmigaOne XE does support 2GB:
https://forum.amiga.org/index.php?topic=54563.0
https://www.amigans.net/modules/newbb/viewtopic.php?viewmode=compact&order=DESC&topic_id=2262&forum=3
(As most other 32bit PPC machines it does not support more as memory above
2GB is reserved for IO.)
The sam460ex does something similar where I think CPU cache can be mapped
as RAM at 0x4_0000_0000 and is used for start up before the memory
controller is initialised by firmware. Maybe something similar is done
here but as we don't emulate the memory controller we just need to make
sure there's some RAM here. Either the system memory is big enough or we
map some RAM for the firmware at 1GB (the same is done in sam460ex as well
but there these aren't overlapping with system RAM so ne need for check
on RAM size there).
>> + mr = g_new(MemoryRegion, 1);
>> + memory_region_init_ram(mr, NULL, "init-cache", 32 * KiB,
>> &error_fatal);
>> + memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
>> + }
>> +
>> + /* allocate and load firmware */
>> + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, fwname);
>> + if (!filename) {
>> + error_report("Could not find firmware '%s'", fwname);
>> + exit(1);
>> + }
>> + rom = g_new(MemoryRegion, 1);
>> + memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
>> + memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
>> + sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE);
>> + if (sz <= 0 || sz > PROM_SIZE) {
>> + error_report("Could not load firmware '%s'", filename);
>> + exit(1);
>> + }
>> + g_free(filename);
>> +
>> + /* Articia S */
>> + dev = sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL);
>> +
>> + i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
>> + if (machine->ram_size > 512 * MiB) {
>> + spd_data = spd_data_generate(SDR, machine->ram_size / 2);
>> + } else {
>> + spd_data = spd_data_generate(SDR, machine->ram_size);
>> + }
>> + fix_spd_data(spd_data);
>> + smbus_eeprom_init_one(i2c_bus, 0x51, spd_data);
>> + if (machine->ram_size > 512 * MiB) {
>> + smbus_eeprom_init_one(i2c_bus, 0x52, spd_data);
>> + }
>
> This seems to confirm my doubts, you use at most 2 SPD of 512MiB DIMMs,
> so max for this machine is 1 GiB.
The firmware has some limitation on DIMM bank size and cannot support 1 GB
bank size, hence we need to fix it up above 512k but it works up to 2g:
$ qemu-system-ppc -M amigaone -m 2g -serial stdio -bios u-boot-amigaone.bin
U-Boot 1.1.1 (Mar 3 2005 - 16:42:53), Build: 03/03/05
CPU: MPC7457 v1.2 @ 1150 MHz
Board: AmigaOne
DRAM:
Information for SIMM bank 0:
Number of banks: 2
Number of row addresses: 14
Number of coumns addresses: 10
SIMM is not registered
Supported burst lenghts: 8 4
Supported CAS latencies: CAS 3
RAS to CAS latency: 2
Precharge latency: 2
SDRAM highest CAS latency: 250
SDRAM 2nd highest CAS latency: 120
SDRAM data width: 8
Auto Refresh supported
Refresh time: 782 clocks
Bank 0 size: 512 MB
Bank 1 size: 512 MB
Information for SIMM bank 1:
Number of banks: 2
Number of row addresses: 14
Number of coumns addresses: 10
SIMM is not registered
Supported burst lenghts: 8 4
Supported CAS latencies: CAS 3
RAS to CAS latency: 2
Precharge latency: 2
SDRAM highest CAS latency: 250
SDRAM 2nd highest CAS latency: 120
SDRAM data width: 8
Auto Refresh supported
Refresh time: 782 clocks
Bank 0 size: 512 MB
Bank 1 size: 512 MB
DIMM0_B0_SCR0 = 0x00000000
DIMM0_B1_SCR0 = 0x00000000
DIMM0_B2_SCR0 = 0x00000000
DIMM0_B3_SCR0 = 0x00000000
Using CAS 4 (slow)
Using CAS 4 (slow)
DRAM_GCR0 = 0x00000000
Refresh set to 1561 clocks, auto refresh on
DRAM_REFRESH0 = 0x00019619
Mode bank 0: 0x00008042
Mode bank 1: 0x00008042
Mode bank 2: 0x00008042
Mode bank 3: 0x00008042
2048 MB
FLASH: 0 kB
*** Warning - bad CRC, using default environment
I plan to rework spd_data_generate again at some point to get rid of these
fix up and to allow machines to better control it (e.g. currently you
can't have 768k RAM which would be a valid config) but for now this works
without touching smbus_eeprom generation so I can look at that later.
(Actually I had that working before in my original spd_eeprom_generate
patches which returned error to the board but later it was "simplified"
and that functionality was lost. As more boards use it now I plan to
restore that but maybe only in next devel cycle as the current one works
for the common cases.)
Regards,
BALATON Zoltan
>> + pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
>> + mr = g_new(MemoryRegion, 1);
>> + memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
>> + 0, 0x1000000);
>> + memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
>> + mr = g_new(MemoryRegion, 1);
>> + memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
>> + 0x80000000, 0x7d000000);
>> + memory_region_add_subregion(get_system_memory(), 0x80000000, mr);
>> + pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>> +
>> + /* VIA VT82c686B South Bridge (multifunction PCI device) */
>> + via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
>> + TYPE_VT82C686B_ISA));
>> + object_property_add_alias(OBJECT(machine), "rtc-time",
>> + object_resolve_path_component(via, "rtc"),
>> + "date");
>> + qdev_connect_gpio_out(DEVICE(via), 0,
>> + qdev_get_gpio_in(DEVICE(cpu),
>> PPC6xx_INPUT_INT));
>> + for (i = 0; i < PCI_NUM_PINS; i++) {
>> + qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
>> + "pirq", i));
>> + }
>> + pci_ide_create_devs(PCI_DEVICE(object_resolve_path_component(via,
>> "ide")));
>> + pci_vga_init(pci_bus);
>> +}
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-09 10:03 ` Bernhard Beschow
@ 2023-10-09 13:02 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-09 13:02 UTC (permalink / raw)
To: Bernhard Beschow
Cc: Mark Cave-Ayland, qemu-devel, qemu-ppc, Nicholas Piggin,
Daniel Henrique Barboza, clg, philmd, Rene Engel, vr_qemu
[-- Attachment #1: Type: text/plain, Size: 8201 bytes --]
On Mon, 9 Oct 2023, Bernhard Beschow wrote:
> Am 8. Oktober 2023 11:08:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>
>>>> The initial value for BARs were set in reset method for emulating
>>>> legacy mode at start but this does not work because PCI code resets
>>>> BARs after calling device reset method.
>>>
>>> This is certainly something I've noticed when testing previous versions of the VIA patches. Perhaps it's worth a separate thread to the PCI devs?
>>
>> I think I brought up this back then but was told current PCI code won't change and since that could break everything else that makes sense so this is something that we should take as given and accomodate that.
>>
>>>> Additionally the values
>>>> written to BARs were also wrong.
>>>
>>> I don't believe this is correct: according to the datasheet the values on reset are the ones given in the current reset code, so even if the reset function is overridden at a later data during PCI bus reset, I would leave these for now since it is a different issue.
>>
>> Those values are missing the IO space bit for one so they can't be correct as a BAR value no matter what the datasheet says. And since they are ineffective now I think it's best to remove them to avoid confusion.
>>
>>>> Move setting the BARs to a callback on writing the PCI config regsiter
>>>> that sets the compatibility mode (which firmwares needing this mode
>>>> seem to do) and fix their values to program it to use legacy port
>>>> numbers. As noted in a comment, we only do this when the BARs were
>>>> unset before, because logs from real machine show this is how real
>>>> chip works, even if it contradicts the data sheet which is not very
>>>> clear about this.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>>>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index fff23803a6..8186190207 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>> PCI_STATUS_DEVSEL_MEDIUM);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>>>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>> }
>>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>>> + uint32_t val, int len)
>>>> +{
>>>> + pci_default_write_config(pd, addr, val, len);
>>>> + /*
>>>> + * Only set BARs if they are unset. Logs from real hardware show that
>>>> + * writing class_prog to enable compatibility mode after BARs were set
>>>> + * (possibly by firmware) it will use ports set by BARs not ISA ports
>>>> + * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
>>>
>>> Can you remind me again where the references are to non-100% native mode? The only thing I can find in Linux is https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360 but that simply forces a switch to legacy mode, with no mention of "non-100% native mode".
>>
>> It was discussed somewhere in the via-ide thread we had when this was last touched for pegasos2 in March 2020. Basically the non-100% native mode is when ports are set by BARs but IRQs are still hard coded to 14-15. Linux can work with all 3 possible modes: legacy (both ports and IRQs are hard coded to ISA values), native (using BARs and PCI config 0x3c for a single interrupt for both channels, vt82c686 data sheet does not document this but vt8231 has a comment saying native mode only) and non-100% native mode where BARs are effective to set port addresses but IRQs don't respect 0x3c but use 14-15 as in legacy mode. Some machines only work in non-100% native mode such as pegasos2 and Linux has some quirks for this. Other OSes running on this machine work with what the firmware has set up and can't work with anything else so we need to emulate what those OSes want (which matches real hardware) because Linux can usually cope anyway. On pegasso2 MorphOS uses BARs but expects IRQ 14-1
5 which is what the firmware also sets up by setting mode to native in the PCI config of the IDE func yet IRQs are fixed at 14-15. Linux forces its driver to use legacy interrupts by setting mode back to legacy but it still uses BARs and this is what it calls non-100% native mode. On amigaone firmware sets legacy mode and AmigaOS does not change this but uses it with legacy ports and IRQs. Linux finds the same and uses legacy mode on amigaone.
>>
>>>> + * But if 0x8a is written after reset without setting BARs then we want
>>>> + * legacy ports (this is done by AmigaOne firmware for example).
>>>> + */
>>>
>>> What should happen here is that writing 0x8a should *always* switch to legacy mode, so the BARs are unused...
>>
>> Yes, but as we've found before that can't be emulated in QEMU due to ISA emulation being static and only allows adding ports but not removing them later so we can't switch between legacy ISA and PCI here so we use the BARs for legacy ports as well to emulate that. The reason we only do this if BARs are not yet set is because Linux changes this back to legacy mode on pegasos2 but still uses BARs
>
> Just curious: How can you tell the difference in real hardware whether
> "raw" IO ports vs. BARs mapped to IO space are used?
Not sure what you mean but if you see ports not matching legacy ports then
you can be fairly sure they are BARs. The chip may internally implement
legacy mode by just fixing BARs at legacy values (the same as this patch
does) but the data sheet does not tell how it works so we don't know that.
>> as shown in boot logs from real hardware
>
> Could you provide links to such logs? That would be very helpful to have
> -- even in the code -- for documentation.
I think if you don't believe me you can just try the emulated pegasos2
with its original firmware and see how it configures the IDE device then
boot Linux with that and see how it detects that. The relevant lines in
the Linux dmesg are:
[ 0.066337] pci 0000:00:0c.1: Fixing VIA IDE, force legacy mode on
[ 0.514351] pata_via 0000:00:0c.1: version 0.3.4
[ 0.536431] scsi0 : pata_via
[ 0.557917] scsi1 : pata_via
[ 0.558249] ata1: PATA max UDMA/100 cmd 0x1000 ctl 0x100c bmdma 0x1020 irq 14
[ 0.558272] ata2: PATA max UDMA/100 cmd 0x1010 ctl 0x101c bmdma 0x1028 irq 15
These are from real machine but emulated one shold do the same with
original firmware. The comments near the IDE fixup in Linux source is
misleading as it says the controller only works in legacy mode but what it
should say is that it uses legacy interrupts even in native mode as seen
from the above logs. The ports are still as mapped by BARs but IRQs aren't
the 9 that the firmware writes to 0x3c PCI config reg for all devices but
14-15 as in legacy mode. This is the non-100% native mode that I think
older Linux versions printed in this case and what's MorphOS and AmigaOS
expect so happen on pegasos2 irrespective of what's in the device tree.
Also the forcing legacy mode quirk in Linux may only edit the device tree
so the driver uses legacy interrupts, I'm not sure, it's been a while I've
looked at this and it was already discussed to death back when I've added
the pegasos2 machine so I don't want to get back to that again. This patch
is tested to allow amigaone to work while not breaking pegasos2 so I'd go
with this for now.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-08 11:08 ` BALATON Zoltan
2023-10-09 9:06 ` Bernhard Beschow
2023-10-09 10:03 ` Bernhard Beschow
@ 2023-10-09 21:29 ` Mark Cave-Ayland
2023-10-09 22:23 ` BALATON Zoltan
2 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-10-09 21:29 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
On 08/10/2023 12:08, BALATON Zoltan wrote:
> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>
>>> The initial value for BARs were set in reset method for emulating
>>> legacy mode at start but this does not work because PCI code resets
>>> BARs after calling device reset method.
>>
>> This is certainly something I've noticed when testing previous versions of the VIA
>> patches. Perhaps it's worth a separate thread to the PCI devs?
>
> I think I brought up this back then but was told current PCI code won't change and
> since that could break everything else that makes sense so this is something that we
> should take as given and accomodate that.
I don't remember the details of that thread, but that's not too much of an issue here
as the values won't be used.
>>> Additionally the values
>>> written to BARs were also wrong.
>>
>> I don't believe this is correct: according to the datasheet the values on reset are
>> the ones given in the current reset code, so even if the reset function is
>> overridden at a later data during PCI bus reset, I would leave these for now since
>> it is a different issue.
>
> Those values are missing the IO space bit for one so they can't be correct as a BAR
> value no matter what the datasheet says. And since they are ineffective now I think
> it's best to remove them to avoid confusion.
Maybe, or perhaps just fix up the missing IO space bit and add a comment pointing out
these are the defaults, but currently they are erased on PCI bus reset? I have found
it useful to have the values around to save having to reference the datasheet.
>>> Move setting the BARs to a callback on writing the PCI config regsiter
>>> that sets the compatibility mode (which firmwares needing this mode
>>> seem to do) and fix their values to program it to use legacy port
>>> numbers. As noted in a comment, we only do this when the BARs were
>>> unset before, because logs from real machine show this is how real
>>> chip works, even if it contradicts the data sheet which is not very
>>> clear about this.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..8186190207 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>> PCI_STATUS_DEVSEL_MEDIUM);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>>> }
>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>> + uint32_t val, int len)
>>> +{
>>> + pci_default_write_config(pd, addr, val, len);
>>> + /*
>>> + * Only set BARs if they are unset. Logs from real hardware show that
>>> + * writing class_prog to enable compatibility mode after BARs were set
>>> + * (possibly by firmware) it will use ports set by BARs not ISA ports
>>> + * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
>>
>> Can you remind me again where the references are to non-100% native mode? The only
>> thing I can find in Linux is
>> https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360 but that simply forces a switch to legacy mode, with no mention of "non-100% native mode".
>
> It was discussed somewhere in the via-ide thread we had when this was last touched
> for pegasos2 in March 2020. Basically the non-100% native mode is when ports are set
> by BARs but IRQs are still hard coded to 14-15. Linux can work with all 3 possible
> modes: legacy (both ports and IRQs are hard coded to ISA values), native (using BARs
> and PCI config 0x3c for a single interrupt for both channels, vt82c686 data sheet
> does not document this but vt8231 has a comment saying native mode only) and non-100%
> native mode where BARs are effective to set port addresses but IRQs don't respect
> 0x3c but use 14-15 as in legacy mode. Some machines only work in non-100% native mode
> such as pegasos2 and Linux has some quirks for this. Other OSes running on this
> machine work with what the firmware has set up and can't work with anything else so
> we need to emulate what those OSes want (which matches real hardware) because Linux
> can usually cope anyway. On pegasso2 MorphOS uses BARs but expects IRQ 14-15 which is
> what the firmware also sets up by setting mode to native in the PCI config of the IDE
> func yet IRQs are fixed at 14-15. Linux forces its driver to use legacy interrupts by
> setting mode back to legacy but it still uses BARs and this is what it calls non-100%
> native mode. On amigaone firmware sets legacy mode and AmigaOS does not change this
> but uses it with legacy ports and IRQs. Linux finds the same and uses legacy mode on
> amigaone.
Thanks for summarising: there have been a number of threads and changes over the
years, so it is easy to lose track of where things are. From the above then
everything except MorphOS that explicitly sets legacy/native mode should just work?
Just to double check I don't think we ever managed to get the PCI configuration
information from real hardware to confirm how the hardware thinks it is set? Is it
possible to dump the PCI config space of the PCI-ISA bridge and the VIA IDE from a
real Pegasos2 booted into Smart Firmware? You can get the information using the Forth
below:
: dump-single-config ( addr )
dup 100 + swap do i u. " : " type i " config-b@" $call-parent u. cr loop
;
" /pci@80000000/isa@C" open-dev to my-self
800 c * 100 0 * + dump-single-config cr cr
800 c * 100 1 * + dump-single-config cr cr
With the corresponding MorphOS debug log that will help to confirm the routing being
used. Also it would be useful to get the output of any MorphOS program that can dump
the current kernel IRQ routing configuration.
The reason for wanting to double-check this now is because my work with the Mac q800
machine showed that both legacy and modern IRQ routing worked, so it would be good to
confirm exactly what IRQ is currently active on real hardware.
>>> + * But if 0x8a is written after reset without setting BARs then we want
>>> + * legacy ports (this is done by AmigaOne firmware for example).
>>> + */
>>
>> What should happen here is that writing 0x8a should *always* switch to legacy mode,
>> so the BARs are unused...
>
> Yes, but as we've found before that can't be emulated in QEMU due to ISA emulation
> being static and only allows adding ports but not removing them later
Fortunately this bug has been fixed, so it should now be possible using
portio_list_*() functions: see
https://gitlab.com/qemu-project/qemu/-/commit/690705ca0b0f1ed24a34ccd14c9866fbe47c69a6.
With that (and a bit of refactoring to allow the sharing of the IDE ioport list) I
was able to switch between compatibility and native modes at will in my PoC. Once
that is working then it doesn't matter if the default BARs aren't set correctly.
> so we can't
> switch between legacy ISA and PCI here so we use the BARs for legacy ports as well to
> emulate that. The reason we only do this if BARs are not yet set is because Linux
> changes this back to legacy mode on pegasos2 but still uses BARs as shown in boot
> logs from real hardware so it seems BARs take priority over legacy mode on real chip
> and Linux only uses the mode reg to decide what IRQs to use. On amigaone firmware
> writes 0x8a right after reset in which case we want legacy mode so this works for
> both machines.
>
>>> + if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>> + pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>> + PCI_BASE_ADDRESS_SPACE_IO) {
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + /* BMIBA: 20-23h */
>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>> + }
>>> +}
>>
>> ... but what you're doing here is effectively forcing the PCI BARs to the legacy
>> addresses. The reason we know this is because that is why you have the off-by-2
>> error in BARs 1 and 3.
>>
>> I could live with this approach for now if you're willing to adjust the comments
>> accordingly clarifying that forcing the PCI BARs to the legacy addresses is a hack
>> to be replaced in future with proper legacy ioports. At some point I will revive my
>> PoC series on top of Bernhard's last series that implements this properly.
>
> That's fair enoough, I can try to clarify thid more in the comments and commit
> message. I'll try to come up with something for v2.
>
> Regards,
> BALATON Zoltan
>
>>> static void via_ide_realize(PCIDevice *dev, Error **errp)
>>> {
>>> PCIIDEState *d = PCI_IDE(dev);
>>> @@ -221,6 +245,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>>> /* Reason: only works as function of VIA southbridge */
>>> dc->user_creatable = false;
>>> + k->config_write = via_ide_cfg_write;
>>> k->realize = via_ide_realize;
>>> k->exit = via_ide_exitfn;
>>> k->vendor_id = PCI_VENDOR_ID_VIA;
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-08 18:08 ` BALATON Zoltan
@ 2023-10-09 21:48 ` Mark Cave-Ayland
2023-10-09 21:57 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-10-09 21:48 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
On 08/10/2023 19:08, BALATON Zoltan wrote:
> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>
>>> The Articia S is a generic chipset supporting several different CPUs
>>> that were used on some PPC boards. This is a minimal emulation of the
>>> parts needed for emulating the AmigaOne board.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/pci-host/Kconfig | 5 +
>>> hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
>>> hw/pci-host/meson.build | 2 +
>>> include/hw/pci-host/articia.h | 17 +++
>>> 4 files changed, 290 insertions(+)
>>> create mode 100644 hw/pci-host/articia.c
>>> create mode 100644 include/hw/pci-host/articia.h
>>>
>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>> index a07070eddf..33014c80a4 100644
>>> --- a/hw/pci-host/Kconfig
>>> +++ b/hw/pci-host/Kconfig
>>> @@ -73,6 +73,11 @@ config SH_PCI
>>> bool
>>> select PCI
>>> +config ARTICIA
>>> + bool
>>> + select PCI
>>> + select I8259
>>> +
>>> config MV64361
>>> bool
>>> select PCI
>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>> new file mode 100644
>>> index 0000000000..80558e1c47
>>> --- /dev/null
>>> +++ b/hw/pci-host/articia.c
>>> @@ -0,0 +1,266 @@
>>> +/*
>>> + * Mai Logic Articia S emulation
>>> + *
>>> + * Copyright (c) 2023 BALATON Zoltan
>>> + *
>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>> + *
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "qapi/error.h"
>>> +#include "hw/pci/pci_device.h"
>>> +#include "hw/pci/pci_host.h"
>>> +#include "hw/irq.h"
>>> +#include "hw/i2c/bitbang_i2c.h"
>>> +#include "hw/intc/i8259.h"
>>> +#include "hw/pci-host/articia.h"
>>> +
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>> +
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>> +struct ArticiaHostState {
>>> + PCIDevice parent_obj;
>>> +
>>> + ArticiaState *as;
>>> +};
>>> +
>>> +/* TYPE_ARTICIA */
>>> +
>>> +struct ArticiaState {
>>> + PCIHostState parent_obj;
>>> +
>>> + qemu_irq irq[PCI_NUM_PINS];
>>> + MemoryRegion io;
>>> + MemoryRegion mem;
>>> + MemoryRegion reg;
>>> +
>>> + bitbang_i2c_interface smbus;
>>> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
>>> + hwaddr gpio_base;
>>> + MemoryRegion gpio_reg;
>>> +};
>>
>> These types above should be in the header file and not in the C file, as per our
>> current QOM guidelines.
>
> I don't think there's such a guideline, at least I did not find any mention of it in
> style and qom docs. It was necessary to move some type declarations to headers for
> types that are embedded in other objects because C needs the struct size for that,
> but I don't think that should be a general thing when it's not needed.
>
> The reason for that is that moving these to the header exposes internal object
> structure to users that should not need to know that so it breaks object
> encapsulation and also needs moving a bunch of includes to the header which then
> makes the users of this type also include those headers when they don't really need
> them but only need the type defines to instantiate the object and that's all they
> should have access to. So I think declaring types in the header should only be done
> for types that aren't full devices and are meant to be embedded as part of another
> device or a SoC but otherwise it's better to keep implementation closed and local to
> the object and not expose it unless really needed, that's why these are here.
>
> If you insist I can move these but I don't think there's really such recommendation
> and I don't think that's a good idea because of the above.
Maybe it was something that was missed out of the recent documentation updates, but
you can clearly see this has been the standard pattern for some time, including for
recent devices such as the xlnx-versal. If there are any devices that don't follow
this pattern then it is likely because they are based on older code.
If you disagree with this, then start a new thread on qemu-devel with a new proposal
and if everyone is agreement then that will be become the new standard.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-09 21:48 ` Mark Cave-Ayland
@ 2023-10-09 21:57 ` BALATON Zoltan
2023-10-09 22:30 ` Mark Cave-Ayland
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-09 21:57 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
[-- Attachment #1: Type: text/plain, Size: 5481 bytes --]
On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
> On 08/10/2023 19:08, BALATON Zoltan wrote:
>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>
>>>> The Articia S is a generic chipset supporting several different CPUs
>>>> that were used on some PPC boards. This is a minimal emulation of the
>>>> parts needed for emulating the AmigaOne board.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/pci-host/Kconfig | 5 +
>>>> hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
>>>> hw/pci-host/meson.build | 2 +
>>>> include/hw/pci-host/articia.h | 17 +++
>>>> 4 files changed, 290 insertions(+)
>>>> create mode 100644 hw/pci-host/articia.c
>>>> create mode 100644 include/hw/pci-host/articia.h
>>>>
>>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>>> index a07070eddf..33014c80a4 100644
>>>> --- a/hw/pci-host/Kconfig
>>>> +++ b/hw/pci-host/Kconfig
>>>> @@ -73,6 +73,11 @@ config SH_PCI
>>>> bool
>>>> select PCI
>>>> +config ARTICIA
>>>> + bool
>>>> + select PCI
>>>> + select I8259
>>>> +
>>>> config MV64361
>>>> bool
>>>> select PCI
>>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>>> new file mode 100644
>>>> index 0000000000..80558e1c47
>>>> --- /dev/null
>>>> +++ b/hw/pci-host/articia.c
>>>> @@ -0,0 +1,266 @@
>>>> +/*
>>>> + * Mai Logic Articia S emulation
>>>> + *
>>>> + * Copyright (c) 2023 BALATON Zoltan
>>>> + *
>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>> + *
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qapi/error.h"
>>>> +#include "hw/pci/pci_device.h"
>>>> +#include "hw/pci/pci_host.h"
>>>> +#include "hw/irq.h"
>>>> +#include "hw/i2c/bitbang_i2c.h"
>>>> +#include "hw/intc/i8259.h"
>>>> +#include "hw/pci-host/articia.h"
>>>> +
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>>> +
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>>> +struct ArticiaHostState {
>>>> + PCIDevice parent_obj;
>>>> +
>>>> + ArticiaState *as;
>>>> +};
>>>> +
>>>> +/* TYPE_ARTICIA */
>>>> +
>>>> +struct ArticiaState {
>>>> + PCIHostState parent_obj;
>>>> +
>>>> + qemu_irq irq[PCI_NUM_PINS];
>>>> + MemoryRegion io;
>>>> + MemoryRegion mem;
>>>> + MemoryRegion reg;
>>>> +
>>>> + bitbang_i2c_interface smbus;
>>>> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1
>>>> out) */
>>>> + hwaddr gpio_base;
>>>> + MemoryRegion gpio_reg;
>>>> +};
>>>
>>> These types above should be in the header file and not in the C file, as
>>> per our current QOM guidelines.
>>
>> I don't think there's such a guideline, at least I did not find any mention
>> of it in style and qom docs. It was necessary to move some type
>> declarations to headers for types that are embedded in other objects
>> because C needs the struct size for that, but I don't think that should be
>> a general thing when it's not needed.
>>
>> The reason for that is that moving these to the header exposes internal
>> object structure to users that should not need to know that so it breaks
>> object encapsulation and also needs moving a bunch of includes to the
>> header which then makes the users of this type also include those headers
>> when they don't really need them but only need the type defines to
>> instantiate the object and that's all they should have access to. So I
>> think declaring types in the header should only be done for types that
>> aren't full devices and are meant to be embedded as part of another device
>> or a SoC but otherwise it's better to keep implementation closed and local
>> to the object and not expose it unless really needed, that's why these are
>> here.
>>
>> If you insist I can move these but I don't think there's really such
>> recommendation and I don't think that's a good idea because of the above.
>
> Maybe it was something that was missed out of the recent documentation
> updates, but you can clearly see this has been the standard pattern for some
> time, including for recent devices such as the xlnx-versal. If there are any
> devices that don't follow this pattern then it is likely because they are
> based on older code.
>
> If you disagree with this, then start a new thread on qemu-devel with a new
> proposal and if everyone is agreement then that will be become the new
> standard.
I think you should start a thread with a patch to style or qom docs about
this to document this standard and if that's accepted then I also accept
it as a real recommendation as my understanding of it was as above that it
was needed for some deviecs to allow embedding them but not a general
recommendation for all devices and I don't think it should be beacuse of
braeaking encapsulation and introduces a lot of unneded includes so I'd
keep it to those devices where it'e really needed which is what the docs
currently say.
But I also said I can change this if you insist as for just this devices
only used once it does not matter much so I take that as you still want
this chnage so I can send another version but wait for the opinion of the
maintainers if they want anything else changed so I cah do all remaining
changes in next version.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-09 21:29 ` Mark Cave-Ayland
@ 2023-10-09 22:23 ` BALATON Zoltan
2023-10-09 22:59 ` Mark Cave-Ayland
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-09 22:23 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
[-- Attachment #1: Type: text/plain, Size: 15455 bytes --]
On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
> On 08/10/2023 12:08, BALATON Zoltan wrote:
>
>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>
>>>> The initial value for BARs were set in reset method for emulating
>>>> legacy mode at start but this does not work because PCI code resets
>>>> BARs after calling device reset method.
>>>
>>> This is certainly something I've noticed when testing previous versions of
>>> the VIA patches. Perhaps it's worth a separate thread to the PCI devs?
>>
>> I think I brought up this back then but was told current PCI code won't
>> change and since that could break everything else that makes sense so this
>> is something that we should take as given and accomodate that.
>
> I don't remember the details of that thread, but that's not too much of an
> issue here as the values won't be used.
>
>>>> Additionally the values
>>>> written to BARs were also wrong.
>>>
>>> I don't believe this is correct: according to the datasheet the values on
>>> reset are the ones given in the current reset code, so even if the reset
>>> function is overridden at a later data during PCI bus reset, I would leave
>>> these for now since it is a different issue.
>>
>> Those values are missing the IO space bit for one so they can't be correct
>> as a BAR value no matter what the datasheet says. And since they are
>> ineffective now I think it's best to remove them to avoid confusion.
>
> Maybe, or perhaps just fix up the missing IO space bit and add a comment
> pointing out these are the defaults, but currently they are erased on PCI bus
> reset? I have found it useful to have the values around to save having to
> reference the datasheet.
The data sheet does not list the io space bits so fixing that would lead
to values not matching data sheet any more. Also the defaults in the data
sheet don't make much sense even with io space but as some of them match
legacy ports while others don't. I can either drop this hunk leaving the
current values there or add a FIXME comment saying they are ineffective
but because they are overwritten (either by PCI code now or firmware/guest
later) I think it's best to remove them any maybe only bring them back if
we find they would be needed for any guest and what would be the correct
default valuss here. I don't trust the data sheet on that and getting it
from real hardware is also not really possible because the firmware could
have overwritten them by the time you can get them. So I don't think
keeping these here would help anybody, just cause confusion.
>>>> Move setting the BARs to a callback on writing the PCI config regsiter
>>>> that sets the compatibility mode (which firmwares needing this mode
>>>> seem to do) and fix their values to program it to use legacy port
>>>> numbers. As noted in a comment, we only do this when the BARs were
>>>> unset before, because logs from real machine show this is how real
>>>> chip works, even if it contradicts the data sheet which is not very
>>>> clear about this.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>>>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index fff23803a6..8186190207 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>> PCI_STATUS_DEVSEL_MEDIUM);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA:
>>>> 20-23h */
>>>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO
>>>> Configuration*/
>>>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>>>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>> }
>>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>>> + uint32_t val, int len)
>>>> +{
>>>> + pci_default_write_config(pd, addr, val, len);
>>>> + /*
>>>> + * Only set BARs if they are unset. Logs from real hardware show
>>>> that
>>>> + * writing class_prog to enable compatibility mode after BARs were
>>>> set
>>>> + * (possibly by firmware) it will use ports set by BARs not ISA
>>>> ports
>>>> + * (e.g. pegasos2 Linux does this and calls it non-100% native
>>>> mode).
>>>
>>> Can you remind me again where the references are to non-100% native mode?
>>> The only thing I can find in Linux is
>>> https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360
>>> but that simply forces a switch to legacy mode, with no mention of
>>> "non-100% native mode".
>>
>> It was discussed somewhere in the via-ide thread we had when this was last
>> touched for pegasos2 in March 2020. Basically the non-100% native mode is
>> when ports are set by BARs but IRQs are still hard coded to 14-15. Linux
>> can work with all 3 possible modes: legacy (both ports and IRQs are hard
>> coded to ISA values), native (using BARs and PCI config 0x3c for a single
>> interrupt for both channels, vt82c686 data sheet does not document this but
>> vt8231 has a comment saying native mode only) and non-100% native mode
>> where BARs are effective to set port addresses but IRQs don't respect 0x3c
>> but use 14-15 as in legacy mode. Some machines only work in non-100% native
>> mode such as pegasos2 and Linux has some quirks for this. Other OSes
>> running on this machine work with what the firmware has set up and can't
>> work with anything else so we need to emulate what those OSes want (which
>> matches real hardware) because Linux can usually cope anyway. On pegasso2
>> MorphOS uses BARs but expects IRQ 14-15 which is what the firmware also
>> sets up by setting mode to native in the PCI config of the IDE func yet
>> IRQs are fixed at 14-15. Linux forces its driver to use legacy interrupts
>> by setting mode back to legacy but it still uses BARs and this is what it
>> calls non-100% native mode. On amigaone firmware sets legacy mode and
>> AmigaOS does not change this but uses it with legacy ports and IRQs. Linux
>> finds the same and uses legacy mode on amigaone.
>
> Thanks for summarising: there have been a number of threads and changes over
> the years, so it is easy to lose track of where things are. From the above
> then everything except MorphOS that explicitly sets legacy/native mode should
> just work?
No, everything but Linux (i.e. MorphOS and AmigaOS) only work with
behaviour matching real hardware which is BARs + legacy interrupts. Linux
may work with other settings but it also has fix ups to detect and work
with the non-100% native mode as on real hardware. (I think this non-100%
native mode was used hy the older Linus via ata driver before it was
ported to libata and became pata_via as I remember seeing these in older
logs but not in newer ones any more which just list IRQs and port numbers
from which you can tell it's neither the legacy nor the native mode
described in the data sheet but a mix between the two.)
> Just to double check I don't think we ever managed to get the PCI
> configuration information from real hardware to confirm how the hardware
> thinks it is set? Is it possible to dump the PCI config space of the PCI-ISA
> bridge and the VIA IDE from a real Pegasos2 booted into Smart Firmware? You
> can get the information using the Forth below:
>
>
> : dump-single-config ( addr )
> dup 100 + swap do i u. " : " type i " config-b@" $call-parent u. cr loop
> ;
>
> " /pci@80000000/isa@C" open-dev to my-self
> 800 c * 100 0 * + dump-single-config cr cr
> 800 c * 100 1 * + dump-single-config cr cr
I don't have dumps from firmware but got an lspci output from Linux from
real pegasos2 which says:
0000:00:0c.1 IDE interface: VIA Technologies, Inc. VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master SecP PriP])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin ? routed to IRQ 14
Region 0: [virtual] I/O ports at 1000 [size=8]
Region 1: [virtual] I/O ports at 100c [size=4]
Region 2: [virtual] I/O ports at 1010 [size=8]
Region 3: [virtual] I/O ports at 101c [size=4]
Region 4: I/O ports at 1020 [size=16]
Capabilities: [c0] Power Management version 2
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Kernel driver in use: pata_via
This says prog_if is 0x8a (probably set by Linux fixup as firmware sets it
to native) but still uses BARs for ports and legacy interrupts although
only says 14 above but in dmesg it listed interruts for the channels as
quoted in reply to Bernhard. Also got /proc infos from same machine that
says:
/proc/ioports: 00001000-00001007 : 0000:00:0c.1
/proc/ioports: 00001000-00001007 : pata_via
/proc/ioports: 0000100c-0000100f : 0000:00:0c.1
/proc/ioports: 0000100c-0000100f : pata_via
/proc/ioports: 00001010-00001017 : 0000:00:0c.1
/proc/ioports: 00001010-00001017 : pata_via
/proc/ioports: 0000101c-0000101f : 0000:00:0c.1
/proc/ioports: 0000101c-0000101f : pata_via
/proc/ioports: 00001020-0000102f : 0000:00:0c.1
/proc/ioports: 00001020-0000102f : pata_via
/proc/interrupts: 14: 1847 i8259 14 Level pata_via
/proc/interrupts: 15: 1210 i8259 15 Level pata_via
So what other proof you still need to believe this is how it works on real
machine?
> With the corresponding MorphOS debug log that will help to confirm the
> routing being used. Also it would be useful to get the output of any MorphOS
> program that can dump the current kernel IRQ routing configuration.
MorphOS does not give much debug logs but it works currenntly as well as
AmigaOS and Linux so what we have now is good enough and this patch does
not break that.
> The reason for wanting to double-check this now is because my work with the
> Mac q800 machine showed that both legacy and modern IRQ routing worked, so it
> would be good to confirm exactly what IRQ is currently active on real
> hardware.
What does q800 has to do with VT8231 and pegaos2? These are totally
different hardware and software designed by separate people so other than
you were looking at both there's no connection to consider between these.
>>>> + * But if 0x8a is written after reset without setting BARs then we
>>>> want
>>>> + * legacy ports (this is done by AmigaOne firmware for example).
>>>> + */
>>>
>>> What should happen here is that writing 0x8a should *always* switch to
>>> legacy mode, so the BARs are unused...
>>
>> Yes, but as we've found before that can't be emulated in QEMU due to ISA
>> emulation being static and only allows adding ports but not removing them
>> later
>
> Fortunately this bug has been fixed, so it should now be possible using
> portio_list_*() functions: see
> https://gitlab.com/qemu-project/qemu/-/commit/690705ca0b0f1ed24a34ccd14c9866fbe47c69a6.
> With that (and a bit of refactoring to allow the sharing of the IDE ioport
> list) I was able to switch between compatibility and native modes at will in
> my PoC. Once that is working then it doesn't matter if the default BARs
> aren't set correctly.
Great but maybe that's not needed because firmware and guests usually
configure this once at boot then use it with that setting without ever
switching it again so this simple patch allows that to work without
breaking what's there and tested already so I'd like this to be merged
first with the amigaone machine then if you want to rewrite it later to
emulate the chip more closely then we have at least two test cases with
pegasos2 and amigaone to verify evertyhing still works correctly. But
since this patch allows all guests to boot it may be a waste of time to
try to patch this emulation further to add functionality that won't be
used by anything. But if it keeps working like this patch does and still
boots the guests this one allows I don't care that much about that.
Regards,
BALATON Zoltan
>> so we can't switch between legacy ISA and PCI here so we use the BARs for
>> legacy ports as well to emulate that. The reason we only do this if BARs
>> are not yet set is because Linux changes this back to legacy mode on
>> pegasos2 but still uses BARs as shown in boot logs from real hardware so it
>> seems BARs take priority over legacy mode on real chip and Linux only uses
>> the mode reg to decide what IRQs to use. On amigaone firmware writes 0x8a
>> right after reset in which case we want legacy mode so this works for both
>> machines.
>>
>>>> + if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>>> + pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>>> + PCI_BASE_ADDRESS_SPACE_IO) {
>>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>>> + /* BMIBA: 20-23h */
>>>> + pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>>> + | PCI_BASE_ADDRESS_SPACE_IO);
>>>> + }
>>>> +}
>>>
>>> ... but what you're doing here is effectively forcing the PCI BARs to the
>>> legacy addresses. The reason we know this is because that is why you have
>>> the off-by-2 error in BARs 1 and 3.
>>>
>>> I could live with this approach for now if you're willing to adjust the
>>> comments accordingly clarifying that forcing the PCI BARs to the legacy
>>> addresses is a hack to be replaced in future with proper legacy ioports.
>>> At some point I will revive my PoC series on top of Bernhard's last series
>>> that implements this properly.
>>
>> That's fair enoough, I can try to clarify thid more in the comments and
>> commit message. I'll try to come up with something for v2.
>>
>> Regards,
>> BALATON Zoltan
>>
>>>> static void via_ide_realize(PCIDevice *dev, Error **errp)
>>>> {
>>>> PCIIDEState *d = PCI_IDE(dev);
>>>> @@ -221,6 +245,7 @@ static void via_ide_class_init(ObjectClass *klass,
>>>> void *data)
>>>> /* Reason: only works as function of VIA southbridge */
>>>> dc->user_creatable = false;
>>>> + k->config_write = via_ide_cfg_write;
>>>> k->realize = via_ide_realize;
>>>> k->exit = via_ide_exitfn;
>>>> k->vendor_id = PCI_VENDOR_ID_VIA;
>
>
> ATB,
>
> Mark.
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-09 21:57 ` BALATON Zoltan
@ 2023-10-09 22:30 ` Mark Cave-Ayland
2023-10-09 22:53 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-10-09 22:30 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
On 09/10/2023 22:57, BALATON Zoltan wrote:
> On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
>> On 08/10/2023 19:08, BALATON Zoltan wrote:
>>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>>
>>>>> The Articia S is a generic chipset supporting several different CPUs
>>>>> that were used on some PPC boards. This is a minimal emulation of the
>>>>> parts needed for emulating the AmigaOne board.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>> hw/pci-host/Kconfig | 5 +
>>>>> hw/pci-host/articia.c | 266 ++++++++++++++++++++++++++++++++++
>>>>> hw/pci-host/meson.build | 2 +
>>>>> include/hw/pci-host/articia.h | 17 +++
>>>>> 4 files changed, 290 insertions(+)
>>>>> create mode 100644 hw/pci-host/articia.c
>>>>> create mode 100644 include/hw/pci-host/articia.h
>>>>>
>>>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>>>> index a07070eddf..33014c80a4 100644
>>>>> --- a/hw/pci-host/Kconfig
>>>>> +++ b/hw/pci-host/Kconfig
>>>>> @@ -73,6 +73,11 @@ config SH_PCI
>>>>> bool
>>>>> select PCI
>>>>> +config ARTICIA
>>>>> + bool
>>>>> + select PCI
>>>>> + select I8259
>>>>> +
>>>>> config MV64361
>>>>> bool
>>>>> select PCI
>>>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>>>> new file mode 100644
>>>>> index 0000000000..80558e1c47
>>>>> --- /dev/null
>>>>> +++ b/hw/pci-host/articia.c
>>>>> @@ -0,0 +1,266 @@
>>>>> +/*
>>>>> + * Mai Logic Articia S emulation
>>>>> + *
>>>>> + * Copyright (c) 2023 BALATON Zoltan
>>>>> + *
>>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include "qemu/osdep.h"
>>>>> +#include "qemu/log.h"
>>>>> +#include "qapi/error.h"
>>>>> +#include "hw/pci/pci_device.h"
>>>>> +#include "hw/pci/pci_host.h"
>>>>> +#include "hw/irq.h"
>>>>> +#include "hw/i2c/bitbang_i2c.h"
>>>>> +#include "hw/intc/i8259.h"
>>>>> +#include "hw/pci-host/articia.h"
>>>>> +
>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>>>> +
>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>>>> +struct ArticiaHostState {
>>>>> + PCIDevice parent_obj;
>>>>> +
>>>>> + ArticiaState *as;
>>>>> +};
>>>>> +
>>>>> +/* TYPE_ARTICIA */
>>>>> +
>>>>> +struct ArticiaState {
>>>>> + PCIHostState parent_obj;
>>>>> +
>>>>> + qemu_irq irq[PCI_NUM_PINS];
>>>>> + MemoryRegion io;
>>>>> + MemoryRegion mem;
>>>>> + MemoryRegion reg;
>>>>> +
>>>>> + bitbang_i2c_interface smbus;
>>>>> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
>>>>> + hwaddr gpio_base;
>>>>> + MemoryRegion gpio_reg;
>>>>> +};
>>>>
>>>> These types above should be in the header file and not in the C file, as per our
>>>> current QOM guidelines.
>>>
>>> I don't think there's such a guideline, at least I did not find any mention of it
>>> in style and qom docs. It was necessary to move some type declarations to headers
>>> for types that are embedded in other objects because C needs the struct size for
>>> that, but I don't think that should be a general thing when it's not needed.
>>>
>>> The reason for that is that moving these to the header exposes internal object
>>> structure to users that should not need to know that so it breaks object
>>> encapsulation and also needs moving a bunch of includes to the header which then
>>> makes the users of this type also include those headers when they don't really
>>> need them but only need the type defines to instantiate the object and that's all
>>> they should have access to. So I think declaring types in the header should only
>>> be done for types that aren't full devices and are meant to be embedded as part of
>>> another device or a SoC but otherwise it's better to keep implementation closed
>>> and local to the object and not expose it unless really needed, that's why these
>>> are here.
>>>
>>> If you insist I can move these but I don't think there's really such
>>> recommendation and I don't think that's a good idea because of the above.
>>
>> Maybe it was something that was missed out of the recent documentation updates, but
>> you can clearly see this has been the standard pattern for some time, including for
>> recent devices such as the xlnx-versal. If there are any devices that don't follow
>> this pattern then it is likely because they are based on older code.
>>
>> If you disagree with this, then start a new thread on qemu-devel with a new
>> proposal and if everyone is agreement then that will be become the new standard.
>
> I think you should start a thread with a patch to style or qom docs about this to
> document this standard and if that's accepted then I also accept it as a real
> recommendation as my understanding of it was as above that it was needed for some
> deviecs to allow embedding them but not a general recommendation for all devices and
> I don't think it should be beacuse of braeaking encapsulation and introduces a lot of
> unneded includes so I'd keep it to those devices where it'e really needed which is
> what the docs currently say.
Oh is there already a mention of this somewhere in the docs? Can you provide a link
so we can check the wording? Certainly that's the way my own patches (and other
people's patches) have been reviewed historically over the years.
> But I also said I can change this if you insist as for just this devices only used
> once it does not matter much so I take that as you still want this chnage so I can
> send another version but wait for the opinion of the maintainers if they want
> anything else changed so I cah do all remaining changes in next version.
Having a separate header would certainly be part of my review criteria as I've been
asked to make such changes in the past. But yeah, maybe wait for a bit and see what
other review comments arrive.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
2023-10-09 22:30 ` Mark Cave-Ayland
@ 2023-10-09 22:53 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-09 22:53 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
[-- Attachment #1: Type: text/plain, Size: 8046 bytes --]
On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
> On 09/10/2023 22:57, BALATON Zoltan wrote:
>> On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
>>> On 08/10/2023 19:08, BALATON Zoltan wrote:
>>>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>>>
>>>>>> The Articia S is a generic chipset supporting several different CPUs
>>>>>> that were used on some PPC boards. This is a minimal emulation of the
>>>>>> parts needed for emulating the AmigaOne board.
>>>>>>
>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>> ---
>>>>>> hw/pci-host/Kconfig | 5 +
>>>>>> hw/pci-host/articia.c | 266
>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>> hw/pci-host/meson.build | 2 +
>>>>>> include/hw/pci-host/articia.h | 17 +++
>>>>>> 4 files changed, 290 insertions(+)
>>>>>> create mode 100644 hw/pci-host/articia.c
>>>>>> create mode 100644 include/hw/pci-host/articia.h
>>>>>>
>>>>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>>>>> index a07070eddf..33014c80a4 100644
>>>>>> --- a/hw/pci-host/Kconfig
>>>>>> +++ b/hw/pci-host/Kconfig
>>>>>> @@ -73,6 +73,11 @@ config SH_PCI
>>>>>> bool
>>>>>> select PCI
>>>>>> +config ARTICIA
>>>>>> + bool
>>>>>> + select PCI
>>>>>> + select I8259
>>>>>> +
>>>>>> config MV64361
>>>>>> bool
>>>>>> select PCI
>>>>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>>>>> new file mode 100644
>>>>>> index 0000000000..80558e1c47
>>>>>> --- /dev/null
>>>>>> +++ b/hw/pci-host/articia.c
>>>>>> @@ -0,0 +1,266 @@
>>>>>> +/*
>>>>>> + * Mai Logic Articia S emulation
>>>>>> + *
>>>>>> + * Copyright (c) 2023 BALATON Zoltan
>>>>>> + *
>>>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qemu/log.h"
>>>>>> +#include "qapi/error.h"
>>>>>> +#include "hw/pci/pci_device.h"
>>>>>> +#include "hw/pci/pci_host.h"
>>>>>> +#include "hw/irq.h"
>>>>>> +#include "hw/i2c/bitbang_i2c.h"
>>>>>> +#include "hw/intc/i8259.h"
>>>>>> +#include "hw/pci-host/articia.h"
>>>>>> +
>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>>>>> +
>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>>>>> +struct ArticiaHostState {
>>>>>> + PCIDevice parent_obj;
>>>>>> +
>>>>>> + ArticiaState *as;
>>>>>> +};
>>>>>> +
>>>>>> +/* TYPE_ARTICIA */
>>>>>> +
>>>>>> +struct ArticiaState {
>>>>>> + PCIHostState parent_obj;
>>>>>> +
>>>>>> + qemu_irq irq[PCI_NUM_PINS];
>>>>>> + MemoryRegion io;
>>>>>> + MemoryRegion mem;
>>>>>> + MemoryRegion reg;
>>>>>> +
>>>>>> + bitbang_i2c_interface smbus;
>>>>>> + uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1
>>>>>> out) */
>>>>>> + hwaddr gpio_base;
>>>>>> + MemoryRegion gpio_reg;
>>>>>> +};
>>>>>
>>>>> These types above should be in the header file and not in the C file, as
>>>>> per our current QOM guidelines.
>>>>
>>>> I don't think there's such a guideline, at least I did not find any
>>>> mention of it in style and qom docs. It was necessary to move some type
>>>> declarations to headers for types that are embedded in other objects
>>>> because C needs the struct size for that, but I don't think that should
>>>> be a general thing when it's not needed.
>>>>
>>>> The reason for that is that moving these to the header exposes internal
>>>> object structure to users that should not need to know that so it breaks
>>>> object encapsulation and also needs moving a bunch of includes to the
>>>> header which then makes the users of this type also include those headers
>>>> when they don't really need them but only need the type defines to
>>>> instantiate the object and that's all they should have access to. So I
>>>> think declaring types in the header should only be done for types that
>>>> aren't full devices and are meant to be embedded as part of another
>>>> device or a SoC but otherwise it's better to keep implementation closed
>>>> and local to the object and not expose it unless really needed, that's
>>>> why these are here.
>>>>
>>>> If you insist I can move these but I don't think there's really such
>>>> recommendation and I don't think that's a good idea because of the above.
>>>
>>> Maybe it was something that was missed out of the recent documentation
>>> updates, but you can clearly see this has been the standard pattern for
>>> some time, including for recent devices such as the xlnx-versal. If there
>>> are any devices that don't follow this pattern then it is likely because
>>> they are based on older code.
>>>
>>> If you disagree with this, then start a new thread on qemu-devel with a
>>> new proposal and if everyone is agreement then that will be become the new
>>> standard.
>>
>> I think you should start a thread with a patch to style or qom docs about
>> this to document this standard and if that's accepted then I also accept it
>> as a real recommendation as my understanding of it was as above that it was
>> needed for some deviecs to allow embedding them but not a general
>> recommendation for all devices and I don't think it should be beacuse of
>> braeaking encapsulation and introduces a lot of unneded includes so I'd
>> keep it to those devices where it'e really needed which is what the docs
>> currently say.
>
> Oh is there already a mention of this somewhere in the docs? Can you provide
> a link so we can check the wording? Certainly that's the way my own patches
> (and other people's patches) have been reviewed historically over the years.
The only mention I could find is in docs/devel/qom.rst, section "Standard
type declaration and definition macros" which says: "In types which do not
require any virtual functions in the class, the OBJECT_DECLARE_SIMPLE_TYPE
macro is suitable, and is commonly placed in the header file" and then
continues as "The 'struct MyDevice' needs to be declared separately." so
it does not say the type should be in the header file, only the
declaration macro and even that is not a requirement just a common
pattern.
As I said above I think the convention of putting typedefs in the header
came as a result of Peter's invention of embedding devices into their
parents that is to avoid tools warning about not freing them which is
reall working around a defficiency in the memory management of QEMU but
this requires the parents to know the size of the objects to embed them as
members so their declaration had to be moved to their public header for
this. But this then breaks object encapsulation, locality and goes against
another effort to reduce the number of unneded includes so I think this
practice should be limited only to cases where it's needed. I've used that
in PPC440 cleanup series before where the SoC object embeds the devices it
contains so these were moved to a public header but here we model a
complete device that isn't meant to be embedded anywhere so the board code
should not need its internal structure. Therefore that's best declared in
the implementation. Moving the OBJECT_DECLARE macros to the header could
be done but also not necessary as per current docs so I wote for keeping
scope of these to only where they are really needed and not expose them
unnecessarily.
Regards,
BALATON Zoltan
>> But I also said I can change this if you insist as for just this devices
>> only used once it does not matter much so I take that as you still want
>> this chnage so I can send another version but wait for the opinion of the
>> maintainers if they want anything else changed so I cah do all remaining
>> changes in next version.
>
> Having a separate header would certainly be part of my review criteria as
> I've been asked to make such changes in the past. But yeah, maybe wait for a
> bit and see what other review comments arrive.
>
>
> ATB,
>
> Mark.
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-09 22:23 ` BALATON Zoltan
@ 2023-10-09 22:59 ` Mark Cave-Ayland
2023-10-10 0:00 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2023-10-09 22:59 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
On 09/10/2023 23:23, BALATON Zoltan wrote:
> On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
>> On 08/10/2023 12:08, BALATON Zoltan wrote:
>>
>>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>>
>>>>> The initial value for BARs were set in reset method for emulating
>>>>> legacy mode at start but this does not work because PCI code resets
>>>>> BARs after calling device reset method.
>>>>
>>>> This is certainly something I've noticed when testing previous versions of the
>>>> VIA patches. Perhaps it's worth a separate thread to the PCI devs?
>>>
>>> I think I brought up this back then but was told current PCI code won't change and
>>> since that could break everything else that makes sense so this is something that
>>> we should take as given and accomodate that.
>>
>> I don't remember the details of that thread, but that's not too much of an issue
>> here as the values won't be used.
>>
>>>>> Additionally the values
>>>>> written to BARs were also wrong.
>>>>
>>>> I don't believe this is correct: according to the datasheet the values on reset
>>>> are the ones given in the current reset code, so even if the reset function is
>>>> overridden at a later data during PCI bus reset, I would leave these for now
>>>> since it is a different issue.
>>>
>>> Those values are missing the IO space bit for one so they can't be correct as a
>>> BAR value no matter what the datasheet says. And since they are ineffective now I
>>> think it's best to remove them to avoid confusion.
>>
>> Maybe, or perhaps just fix up the missing IO space bit and add a comment pointing
>> out these are the defaults, but currently they are erased on PCI bus reset? I have
>> found it useful to have the values around to save having to reference the datasheet.
>
> The data sheet does not list the io space bits so fixing that would lead to values
> not matching data sheet any more. Also the defaults in the data sheet don't make much
> sense even with io space but as some of them match legacy ports while others don't. I
> can either drop this hunk leaving the current values there or add a FIXME comment
> saying they are ineffective but because they are overwritten (either by PCI code now
> or firmware/guest later) I think it's best to remove them any maybe only bring them
> back if we find they would be needed for any guest and what would be the correct
> default valuss here. I don't trust the data sheet on that and getting it from real
> hardware is also not really possible because the firmware could have overwritten them
> by the time you can get them. So I don't think keeping these here would help anybody,
> just cause confusion.
We can check the values on real hardware given the Forth in my previous reply which
will then tell us the correct values for once and for all. My guess is that since the
address is a separate field to the BAR type in the datasheet, the IO bit was simply
missed.
>>>>> Move setting the BARs to a callback on writing the PCI config regsiter
>>>>> that sets the compatibility mode (which firmwares needing this mode
>>>>> seem to do) and fix their values to program it to use legacy port
>>>>> numbers. As noted in a comment, we only do this when the BARs were
>>>>> unset before, because logs from real machine show this is how real
>>>>> chip works, even if it contradicts the data sheet which is not very
>>>>> clear about this.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>>>>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>>> index fff23803a6..8186190207 100644
>>>>> --- a/hw/ide/via.c
>>>>> +++ b/hw/ide/via.c
>>>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>>> PCI_STATUS_DEVSEL_MEDIUM);
>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>>>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>>>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>>>>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>>> }
>>>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>>>> + uint32_t val, int len)
>>>>> +{
>>>>> + pci_default_write_config(pd, addr, val, len);
>>>>> + /*
>>>>> + * Only set BARs if they are unset. Logs from real hardware show that
>>>>> + * writing class_prog to enable compatibility mode after BARs were set
>>>>> + * (possibly by firmware) it will use ports set by BARs not ISA ports
>>>>> + * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
>>>>
>>>> Can you remind me again where the references are to non-100% native mode? The
>>>> only thing I can find in Linux is
>>>> https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360 but that simply forces a switch to legacy mode, with no mention of "non-100% native mode".
>>>
>>> It was discussed somewhere in the via-ide thread we had when this was last touched
>>> for pegasos2 in March 2020. Basically the non-100% native mode is when ports are
>>> set by BARs but IRQs are still hard coded to 14-15. Linux can work with all 3
>>> possible modes: legacy (both ports and IRQs are hard coded to ISA values), native
>>> (using BARs and PCI config 0x3c for a single interrupt for both channels, vt82c686
>>> data sheet does not document this but vt8231 has a comment saying native mode
>>> only) and non-100% native mode where BARs are effective to set port addresses but
>>> IRQs don't respect 0x3c but use 14-15 as in legacy mode. Some machines only work
>>> in non-100% native mode such as pegasos2 and Linux has some quirks for this. Other
>>> OSes running on this machine work with what the firmware has set up and can't work
>>> with anything else so we need to emulate what those OSes want (which matches real
>>> hardware) because Linux can usually cope anyway. On pegasso2 MorphOS uses BARs but
>>> expects IRQ 14-15 which is what the firmware also sets up by setting mode to
>>> native in the PCI config of the IDE func yet IRQs are fixed at 14-15. Linux forces
>>> its driver to use legacy interrupts by setting mode back to legacy but it still
>>> uses BARs and this is what it calls non-100% native mode. On amigaone firmware
>>> sets legacy mode and AmigaOS does not change this but uses it with legacy ports
>>> and IRQs. Linux finds the same and uses legacy mode on amigaone.
>>
>> Thanks for summarising: there have been a number of threads and changes over the
>> years, so it is easy to lose track of where things are. From the above then
>> everything except MorphOS that explicitly sets legacy/native mode should just work?
>
> No, everything but Linux (i.e. MorphOS and AmigaOS) only work with behaviour matching
> real hardware which is BARs + legacy interrupts. Linux may work with other settings
> but it also has fix ups to detect and work with the non-100% native mode as on real
> hardware. (I think this non-100% native mode was used hy the older Linus via ata
> driver before it was ported to libata and became pata_via as I remember seeing these
> in older logs but not in newer ones any more which just list IRQs and port numbers
> from which you can tell it's neither the legacy nor the native mode described in the
> data sheet but a mix between the two.)
>
>> Just to double check I don't think we ever managed to get the PCI configuration
>> information from real hardware to confirm how the hardware thinks it is set? Is it
>> possible to dump the PCI config space of the PCI-ISA bridge and the VIA IDE from a
>> real Pegasos2 booted into Smart Firmware? You can get the information using the
>> Forth below:
>>
>>
>> : dump-single-config ( addr )
>> dup 100 + swap do i u. " : " type i " config-b@" $call-parent u. cr loop
>> ;
>>
>> " /pci@80000000/isa@C" open-dev to my-self
>> 800 c * 100 0 * + dump-single-config cr cr
>> 800 c * 100 1 * + dump-single-config cr cr
>
> I don't have dumps from firmware but got an lspci output from Linux from real
> pegasos2 which says:
Ah no that won't work because Linux has already made changes to PCI configuration
space. What we need is the value from the firmware to confirm exactly how the
hardware has been configured before the kernel has loaded (as you mentioned above).
Who can we ask to get this information?
> 0000:00:0c.1 IDE interface: VIA Technologies, Inc.
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master
> SecP PriP])
> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
> <MAbort- >SERR- <PERR- INTx-
> Latency: 0
> Interrupt: pin ? routed to IRQ 14
Now this is interesting - I wonder what "?" means here? Perhaps the switch to
compatibility mode has set the PCI interrupt pin to 0? Oh and what kernel version is
this?
> Region 0: [virtual] I/O ports at 1000 [size=8]
> Region 1: [virtual] I/O ports at 100c [size=4]
> Region 2: [virtual] I/O ports at 1010 [size=8]
> Region 3: [virtual] I/O ports at 101c [size=4]
> Region 4: I/O ports at 1020 [size=16]
> Capabilities: [c0] Power Management version 2
> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
> PME(D0-,D1-,D2-,D3hot-,D3cold-)
> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
> Kernel driver in use: pata_via
>
> This says prog_if is 0x8a (probably set by Linux fixup as firmware sets it to native)
> but still uses BARs for ports and legacy interrupts although only says 14 above but
> in dmesg it listed interruts for the channels as quoted in reply to Bernhard. Also
> got /proc infos from same machine that says:
>
> /proc/ioports: 00001000-00001007 : 0000:00:0c.1
> /proc/ioports: 00001000-00001007 : pata_via
> /proc/ioports: 0000100c-0000100f : 0000:00:0c.1
> /proc/ioports: 0000100c-0000100f : pata_via
> /proc/ioports: 00001010-00001017 : 0000:00:0c.1
> /proc/ioports: 00001010-00001017 : pata_via
> /proc/ioports: 0000101c-0000101f : 0000:00:0c.1
> /proc/ioports: 0000101c-0000101f : pata_via
> /proc/ioports: 00001020-0000102f : 0000:00:0c.1
> /proc/ioports: 00001020-0000102f : pata_via
Interesting. The PCI-IDE specification states that the BAR values are ignored in
compatibility mode, so it could be that they contain values left over from the
firmware they are not being actively decoded on the bus. They certainly look similar
to the values set by the firmware.
In fact it would be possible to write some Forth to switch to compatibility mode in
the firmware and then try and access the BARs to confirm whether or not the BARs are
really being decoded.
> /proc/interrupts: 14: 1847 i8259 14 Level pata_via
> /proc/interrupts: 15: 1210 i8259 15 Level pata_via
>
> So what other proof you still need to believe this is how it works on real machine?
Again this would have been overwritten by the switch to compatibility mode.
>> With the corresponding MorphOS debug log that will help to confirm the routing
>> being used. Also it would be useful to get the output of any MorphOS program that
>> can dump the current kernel IRQ routing configuration.
>
> MorphOS does not give much debug logs but it works currenntly as well as AmigaOS and
> Linux so what we have now is good enough and this patch does not break that.
It looks like you can get PCI debug logs, at least that seems work here on QEMU. They
would be very useful to understand the exact behaviour.
>> The reason for wanting to double-check this now is because my work with the Mac
>> q800 machine showed that both legacy and modern IRQ routing worked, so it would be
>> good to confirm exactly what IRQ is currently active on real hardware.
>
> What does q800 has to do with VT8231 and pegaos2? These are totally different
> hardware and software designed by separate people so other than you were looking at
> both there's no connection to consider between these.
The point I was making was that there may be multiple IRQ routing possibilities for
the IDE controller, in which case it makes sense to be absolutely sure which one is
being used on real hardware.
>>>>> + * But if 0x8a is written after reset without setting BARs then we want
>>>>> + * legacy ports (this is done by AmigaOne firmware for example).
>>>>> + */
>>>>
>>>> What should happen here is that writing 0x8a should *always* switch to legacy
>>>> mode, so the BARs are unused...
>>>
>>> Yes, but as we've found before that can't be emulated in QEMU due to ISA emulation
>>> being static and only allows adding ports but not removing them later
>>
>> Fortunately this bug has been fixed, so it should now be possible using
>> portio_list_*() functions: see
>> https://gitlab.com/qemu-project/qemu/-/commit/690705ca0b0f1ed24a34ccd14c9866fbe47c69a6. With that (and a bit of refactoring to allow the sharing of the IDE ioport list) I was able to switch between compatibility and native modes at will in my PoC. Once that is working then it doesn't matter if the default BARs aren't set correctly.
>
> Great but maybe that's not needed because firmware and guests usually configure this
> once at boot then use it with that setting without ever switching it again so this
> simple patch allows that to work without breaking what's there and tested already so
> I'd like this to be merged first with the amigaone machine then if you want to
> rewrite it later to emulate the chip more closely then we have at least two test
> cases with pegasos2 and amigaone to verify evertyhing still works correctly. But
> since this patch allows all guests to boot it may be a waste of time to try to patch
> this emulation further to add functionality that won't be used by anything. But if it
> keeps working like this patch does and still boots the guests this one allows I don't
> care that much about that.
The ultimate aim is to combine mine and Bernhard's work so that the compatibility
mode/PCI native mode switch is handle automatically by the PCI IDE core, so it's
something that would not just be restricted to the VIA.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
2023-10-09 22:59 ` Mark Cave-Ayland
@ 2023-10-10 0:00 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2023-10-10 0:00 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, qemu-ppc, Nicholas Piggin, Daniel Henrique Barboza,
clg, philmd, Bernhard Beschow, Rene Engel, vr_qemu
[-- Attachment #1: Type: text/plain, Size: 20540 bytes --]
On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
> On 09/10/2023 23:23, BALATON Zoltan wrote:
>> On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
>>> On 08/10/2023 12:08, BALATON Zoltan wrote:
>>>
>>>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>>>
>>>>>> The initial value for BARs were set in reset method for emulating
>>>>>> legacy mode at start but this does not work because PCI code resets
>>>>>> BARs after calling device reset method.
>>>>>
>>>>> This is certainly something I've noticed when testing previous versions
>>>>> of the VIA patches. Perhaps it's worth a separate thread to the PCI
>>>>> devs?
>>>>
>>>> I think I brought up this back then but was told current PCI code won't
>>>> change and since that could break everything else that makes sense so
>>>> this is something that we should take as given and accomodate that.
>>>
>>> I don't remember the details of that thread, but that's not too much of an
>>> issue here as the values won't be used.
>>>
>>>>>> Additionally the values
>>>>>> written to BARs were also wrong.
>>>>>
>>>>> I don't believe this is correct: according to the datasheet the values
>>>>> on reset are the ones given in the current reset code, so even if the
>>>>> reset function is overridden at a later data during PCI bus reset, I
>>>>> would leave these for now since it is a different issue.
>>>>
>>>> Those values are missing the IO space bit for one so they can't be
>>>> correct as a BAR value no matter what the datasheet says. And since they
>>>> are ineffective now I think it's best to remove them to avoid confusion.
>>>
>>> Maybe, or perhaps just fix up the missing IO space bit and add a comment
>>> pointing out these are the defaults, but currently they are erased on PCI
>>> bus reset? I have found it useful to have the values around to save having
>>> to reference the datasheet.
>>
>> The data sheet does not list the io space bits so fixing that would lead to
>> values not matching data sheet any more. Also the defaults in the data
>> sheet don't make much sense even with io space but as some of them match
>> legacy ports while others don't. I can either drop this hunk leaving the
>> current values there or add a FIXME comment saying they are ineffective but
>> because they are overwritten (either by PCI code now or firmware/guest
>> later) I think it's best to remove them any maybe only bring them back if
>> we find they would be needed for any guest and what would be the correct
>> default valuss here. I don't trust the data sheet on that and getting it
>> from real hardware is also not really possible because the firmware could
>> have overwritten them by the time you can get them. So I don't think
>> keeping these here would help anybody, just cause confusion.
>
> We can check the values on real hardware given the Forth in my previous reply
> which will then tell us the correct values for once and for all. My guess is
> that since the address is a separate field to the BAR type in the datasheet,
> the IO bit was simply missed.
I only have device tree dump from real pegasos2 but it has BARs so you
can see here:
package 0xFC5CE70 /pci/ide:
parent=0xFC58E08
children=0xFC5D418
link=0xFC5D948
dict=0xFC5CEA0:
open func[0x11F4]
close func[0x11F5]
dma-alloc func[0x11F6]
dma-free func[0x11F7]
decode-unit func[0x11F8]
encode-unit func[0x11F9]
selftest func[0x11FA]
props=0xFC5CEB8:
vendor-id 0x1106 (4358)
device-id 0x571 (1393)
revision-id 0x6 (6)
class-code 0x1018F (65935)
subsystem-id 0x0 (0)
subsystem-vendor-id 0x0 (0)
.vendor-name "VIA"
.part-number "VT82C586/596/686"
.description "PCI IDE Controller"
interrupts 0x1 (1)
devsel-speed 0x1 (1)
fast-back-to-back
min-grant 0x0 (0)
max-latency 0x0 (0)
name "ide"
reg C,1:0
iC,1,10,0:8
iC,1,14,0:4
iC,1,18,0:8
iC,1,1C,0:4
iC,1,20,0:10
device_type "spi"
assigned-addresses iC,1,10,FE001000:8
iC,1,14,FE00100C:4
iC,1,18,FE001010:8
iC,1,1C,FE00101C:4
iC,1,20,FE001020:10
That means by the time you could run your Forth the firmware already set
the IDE device up se we get the same values as in Linux. If you want to
see how the firmware progrems it you can find out with the emulated
pegasos2 using -bios pegasos2.rom and enabling pci traces, but we can't
find out what are the default values on real machine before this without
replacing firmware that likely nobody dares to try. So just let it go and
accept that whatever defaults are there, they don't matter because the
firmware on pegasos2 overwrites them and the firmware on amigaone sets
legacy mode so it should use legacy ports there and if anything switches
to native mode later then it will also program BARs so it really does not
matter what the data sheet says, what real hardware does and weather we
put any default valiues there in QEMU or not (especially with PCI code
resetting it to 0 now anyway). So I think the way to go is to get rid of
these lines and don't bother with it until we find something that depends
on defaults but that's very unlilkely as even in PCs the BIOS would
program it or if you boot an OS then that will either use legacy mode or
it programs the device as a PCI deviec and sets BARs. I can't imagine
anything just setting native mode and then not change BARs and on real
hardware it can't do that before firmware that most likely has already
overwritten defaults according to BIOS setup where you can usually select
native or legacy mode so what we need to match here is how it is after
firmware.
>>>>>> Move setting the BARs to a callback on writing the PCI config regsiter
>>>>>> that sets the compatibility mode (which firmwares needing this mode
>>>>>> seem to do) and fix their values to program it to use legacy port
>>>>>> numbers. As noted in a comment, we only do this when the BARs were
>>>>>> unset before, because logs from real machine show this is how real
>>>>>> chip works, even if it contradicts the data sheet which is not very
>>>>>> clear about this.
>>>>>>
>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>> ---
>>>>>> hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>>>>>> 1 file changed, 30 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>>>> index fff23803a6..8186190207 100644
>>>>>> --- a/hw/ide/via.c
>>>>>> +++ b/hw/ide/via.c
>>>>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>>>> PCI_STATUS_DEVSEL_MEDIUM);
>>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>>>> - pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA:
>>>>>> 20-23h */
>>>>>> pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>>>> /* IDE chip enable, IDE configuration 1/2, IDE FIFO
>>>>>> Configuration*/
>>>>>> @@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
>>>>>> pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>>>> }
>>>>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>>>>> + uint32_t val, int len)
>>>>>> +{
>>>>>> + pci_default_write_config(pd, addr, val, len);
>>>>>> + /*
>>>>>> + * Only set BARs if they are unset. Logs from real hardware show
>>>>>> that
>>>>>> + * writing class_prog to enable compatibility mode after BARs were
>>>>>> set
>>>>>> + * (possibly by firmware) it will use ports set by BARs not ISA
>>>>>> ports
>>>>>> + * (e.g. pegasos2 Linux does this and calls it non-100% native
>>>>>> mode).
>>>>>
>>>>> Can you remind me again where the references are to non-100% native
>>>>> mode? The only thing I can find in Linux is
>>>>> https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360
>>>>> but that simply forces a switch to legacy mode, with no mention of
>>>>> "non-100% native mode".
>>>>
>>>> It was discussed somewhere in the via-ide thread we had when this was
>>>> last touched for pegasos2 in March 2020. Basically the non-100% native
>>>> mode is when ports are set by BARs but IRQs are still hard coded to
>>>> 14-15. Linux can work with all 3 possible modes: legacy (both ports and
>>>> IRQs are hard coded to ISA values), native (using BARs and PCI config
>>>> 0x3c for a single interrupt for both channels, vt82c686 data sheet does
>>>> not document this but vt8231 has a comment saying native mode only) and
>>>> non-100% native mode where BARs are effective to set port addresses but
>>>> IRQs don't respect 0x3c but use 14-15 as in legacy mode. Some machines
>>>> only work in non-100% native mode such as pegasos2 and Linux has some
>>>> quirks for this. Other OSes running on this machine work with what the
>>>> firmware has set up and can't work with anything else so we need to
>>>> emulate what those OSes want (which matches real hardware) because Linux
>>>> can usually cope anyway. On pegasso2 MorphOS uses BARs but expects IRQ
>>>> 14-15 which is what the firmware also sets up by setting mode to native
>>>> in the PCI config of the IDE func yet IRQs are fixed at 14-15. Linux
>>>> forces its driver to use legacy interrupts by setting mode back to legacy
>>>> but it still uses BARs and this is what it calls non-100% native mode. On
>>>> amigaone firmware sets legacy mode and AmigaOS does not change this but
>>>> uses it with legacy ports and IRQs. Linux finds the same and uses legacy
>>>> mode on amigaone.
>>>
>>> Thanks for summarising: there have been a number of threads and changes
>>> over the years, so it is easy to lose track of where things are. From the
>>> above then everything except MorphOS that explicitly sets legacy/native
>>> mode should just work?
>>
>> No, everything but Linux (i.e. MorphOS and AmigaOS) only work with
>> behaviour matching real hardware which is BARs + legacy interrupts. Linux
>> may work with other settings but it also has fix ups to detect and work
>> with the non-100% native mode as on real hardware. (I think this non-100%
>> native mode was used hy the older Linus via ata driver before it was ported
>> to libata and became pata_via as I remember seeing these in older logs but
>> not in newer ones any more which just list IRQs and port numbers from which
>> you can tell it's neither the legacy nor the native mode described in the
>> data sheet but a mix between the two.)
>>
>>> Just to double check I don't think we ever managed to get the PCI
>>> configuration information from real hardware to confirm how the hardware
>>> thinks it is set? Is it possible to dump the PCI config space of the
>>> PCI-ISA bridge and the VIA IDE from a real Pegasos2 booted into Smart
>>> Firmware? You can get the information using the Forth below:
>>>
>>>
>>> : dump-single-config ( addr )
>>> dup 100 + swap do i u. " : " type i " config-b@" $call-parent u. cr loop
>>> ;
>>>
>>> " /pci@80000000/isa@C" open-dev to my-self
>>> 800 c * 100 0 * + dump-single-config cr cr
>>> 800 c * 100 1 * + dump-single-config cr cr
>>
>> I don't have dumps from firmware but got an lspci output from Linux from
>> real pegasos2 which says:
>
> Ah no that won't work because Linux has already made changes to PCI
> configuration space. What we need is the value from the firmware to confirm
> exactly how the hardware has been configured before the kernel has loaded (as
> you mentioned above). Who can we ask to get this information?
>
>> 0000:00:0c.1 IDE interface: VIA Technologies, Inc.
>> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if
>> 8a [Master SecP PriP])
>> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
>> ParErr- Stepping- SERR- FastB2B- DisINTx-
>> Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
>> <TAbort- <MAbort- >SERR- <PERR- INTx-
>> Latency: 0
>> Interrupt: pin ? routed to IRQ 14
>
> Now this is interesting - I wonder what "?" means here? Perhaps the switch to
> compatibility mode has set the PCI interrupt pin to 0? Oh and what kernel
> version is this?
I think:
[ 0.000000] Linux version 3.16.0-6-powerpc (debian-kernel@lists.debian.org) (gcc version 4.8.4 (Debian 4.8.4-1) ) #1 Debian 3.16.56-1+deb8u1 (2018-05-08)
but I got logs in different files so not entirely sure.
>> Region 0: [virtual] I/O ports at 1000 [size=8]
>> Region 1: [virtual] I/O ports at 100c [size=4]
>> Region 2: [virtual] I/O ports at 1010 [size=8]
>> Region 3: [virtual] I/O ports at 101c [size=4]
>> Region 4: I/O ports at 1020 [size=16]
>> Capabilities: [c0] Power Management version 2
>> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
>> PME(D0-,D1-,D2-,D3hot-,D3cold-)
>> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
>> Kernel driver in use: pata_via
>>
>> This says prog_if is 0x8a (probably set by Linux fixup as firmware sets it
>> to native) but still uses BARs for ports and legacy interrupts although
>> only says 14 above but in dmesg it listed interruts for the channels as
>> quoted in reply to Bernhard. Also got /proc infos from same machine that
>> says:
>>
>> /proc/ioports: 00001000-00001007 : 0000:00:0c.1
>> /proc/ioports: 00001000-00001007 : pata_via
>> /proc/ioports: 0000100c-0000100f : 0000:00:0c.1
>> /proc/ioports: 0000100c-0000100f : pata_via
>> /proc/ioports: 00001010-00001017 : 0000:00:0c.1
>> /proc/ioports: 00001010-00001017 : pata_via
>> /proc/ioports: 0000101c-0000101f : 0000:00:0c.1
>> /proc/ioports: 0000101c-0000101f : pata_via
>> /proc/ioports: 00001020-0000102f : 0000:00:0c.1
>> /proc/ioports: 00001020-0000102f : pata_via
>
> Interesting. The PCI-IDE specification states that the BAR values are ignored
> in compatibility mode, so it could be that they contain values left over from
> the firmware they are not being actively decoded on the bus. They certainly
> look similar to the values set by the firmware.
These are the values set by the firmware as you can see from the device
tree dump above.
> In fact it would be possible to write some Forth to switch to compatibility
> mode in the firmware and then try and access the BARs to confirm whether or
> not the BARs are really being decoded.
So Linux does that on boot by writing 0x8a to prog_if when forcing legacy
mode in fix up and then still uses BARs so I think that confirms it works
like that even in legacy mode unless the BARs are unset which is what
happens on the amigaone that only writes 0x8a even though that should be
the default (again you can check this with the emulated machine).
>> /proc/interrupts: 14: 1847 i8259 14 Level pata_via
>> /proc/interrupts: 15: 1210 i8259 15 Level pata_via
>>
>> So what other proof you still need to believe this is how it works on real
>> machine?
>
> Again this would have been overwritten by the switch to compatibility mode.
But I think AmigaOS and MorphOS do not switch to legacy mode and still use
BARs and legacy IRQs as set by the firmware. These now boot so our device
model is matching real hardware well enough.
>>> With the corresponding MorphOS debug log that will help to confirm the
>>> routing being used. Also it would be useful to get the output of any
>>> MorphOS program that can dump the current kernel IRQ routing
>>> configuration.
>>
>> MorphOS does not give much debug logs but it works currenntly as well as
>> AmigaOS and Linux so what we have now is good enough and this patch does
>> not break that.
>
> It looks like you can get PCI debug logs, at least that seems work here on
> QEMU. They would be very useful to understand the exact behaviour.
>
>>> The reason for wanting to double-check this now is because my work with
>>> the Mac q800 machine showed that both legacy and modern IRQ routing
>>> worked, so it would be good to confirm exactly what IRQ is currently
>>> active on real hardware.
>>
>> What does q800 has to do with VT8231 and pegaos2? These are totally
>> different hardware and software designed by separate people so other than
>> you were looking at both there's no connection to consider between these.
>
> The point I was making was that there may be multiple IRQ routing
> possibilities for the IDE controller, in which case it makes sense to be
> absolutely sure which one is being used on real hardware.
>
>>>>>> + * But if 0x8a is written after reset without setting BARs then we
>>>>>> want
>>>>>> + * legacy ports (this is done by AmigaOne firmware for example).
>>>>>> + */
>>>>>
>>>>> What should happen here is that writing 0x8a should *always* switch to
>>>>> legacy mode, so the BARs are unused...
>>>>
>>>> Yes, but as we've found before that can't be emulated in QEMU due to ISA
>>>> emulation being static and only allows adding ports but not removing them
>>>> later
>>>
>>> Fortunately this bug has been fixed, so it should now be possible using
>>> portio_list_*() functions: see
>>> https://gitlab.com/qemu-project/qemu/-/commit/690705ca0b0f1ed24a34ccd14c9866fbe47c69a6.
>>> With that (and a bit of refactoring to allow the sharing of the IDE ioport
>>> list) I was able to switch between compatibility and native modes at will
>>> in my PoC. Once that is working then it doesn't matter if the default BARs
>>> aren't set correctly.
>>
>> Great but maybe that's not needed because firmware and guests usually
>> configure this once at boot then use it with that setting without ever
>> switching it again so this simple patch allows that to work without
>> breaking what's there and tested already so I'd like this to be merged
>> first with the amigaone machine then if you want to rewrite it later to
>> emulate the chip more closely then we have at least two test cases with
>> pegasos2 and amigaone to verify evertyhing still works correctly. But since
>> this patch allows all guests to boot it may be a waste of time to try to
>> patch this emulation further to add functionality that won't be used by
>> anything. But if it keeps working like this patch does and still boots the
>> guests this one allows I don't care that much about that.
>
> The ultimate aim is to combine mine and Bernhard's work so that the
> compatibility mode/PCI native mode switch is handle automatically by the PCI
> IDE core, so it's something that would not just be restricted to the VIA.
OK but that should be done separately and not intermixed/blocking this
series as this model can later be changed but I'd like to get amigaone
working now so a later goal should not prevent that now.
I'm not sure your goal can be achieved though because these different
south bridges may have some differences so not sure they can be handled
the same way. I'd expect Intel PIIX models to be similar and VIA chips be
similar so these can share code within the same family but just as Intel
ac97 is different than VIA ac97 their IDE parts may also not behave the
same so these may need to be modelled separately. We can try to find that
out as long as it does not block people using these machines now.
Regards,
BALATON Zoltan
> ATB,
>
> Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-10-10 0:02 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-05 22:12 [PATCH 0/3] Add emulation of AmigaOne XE board BALATON Zoltan
2023-10-05 22:13 ` [PATCH 1/3] via-ide: Fix legacy mode emulation BALATON Zoltan
2023-10-08 6:13 ` Mark Cave-Ayland
2023-10-08 11:08 ` BALATON Zoltan
2023-10-09 9:06 ` Bernhard Beschow
2023-10-09 11:22 ` BALATON Zoltan
2023-10-09 10:03 ` Bernhard Beschow
2023-10-09 13:02 ` BALATON Zoltan
2023-10-09 21:29 ` Mark Cave-Ayland
2023-10-09 22:23 ` BALATON Zoltan
2023-10-09 22:59 ` Mark Cave-Ayland
2023-10-10 0:00 ` BALATON Zoltan
2023-10-05 22:13 ` [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S BALATON Zoltan
2023-10-06 21:13 ` Volker Rümelin
2023-10-07 10:21 ` BALATON Zoltan
2023-10-08 6:14 ` Mark Cave-Ayland
2023-10-08 18:08 ` BALATON Zoltan
2023-10-09 21:48 ` Mark Cave-Ayland
2023-10-09 21:57 ` BALATON Zoltan
2023-10-09 22:30 ` Mark Cave-Ayland
2023-10-09 22:53 ` BALATON Zoltan
2023-10-05 22:13 ` [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board BALATON Zoltan
2023-10-09 6:24 ` Philippe Mathieu-Daudé
2023-10-09 11:56 ` BALATON Zoltan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.