Re: [PATCH] hw/ppc/e500: Partial implementation of local access window registers

[BALATON Zoltan <balaton@eik.bme.hu>]

On Sat, 1 Feb 2025, Bernhard Beschow wrote:
> Am 1. Februar 2025 14:55:15 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> Am 30. Januar 2025 12:45:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Wed, 15 Jan 2025, BALATON Zoltan wrote:
>>>> This allows guests to set the CCSR base address. Also store and return
>>>> values of the local access window registers but their functionality
>>>> isn't implemented.
>>>
>>> Ping?
>>
>> I guess you're trying to boot a real firmware image from SD card?

I'm not trying, I've done it. I only needed these patches and commenting 
out the page_aligned = true in hw/sd/sdhci.c as I noted in the previous 
patch. U-Boot works and I can run commands in the firmware prompt but I 
did not try to boot an OS yet. The amigaos boot loader which U-Boot for 
Tabor loads by default crashes somewhere but this may be a bug in the 
patched U-Boot. I think I've seen similar with sam460ex U-Boot before, 
maybe it's because of not finding some expected devices and not handling 
the returned NULL value well but I did not debug it.

>> I've implemented that in my e500-fdt branch which I want to send as an 
>> RFC. I still need to clean it up. Once it's on the list we could make a 
>> plan how to turn it into a p10xx. Would that work for you?

Sure, I can try to test your patches once they are submitted to the list 
and rebase my changes on top if they still needed. I've just submitted 
these so you can incorporate them in your tree so I have less to rebase 
but I see you already have most of these. I'm OK to wait until your tree 
is cleaned and submitted but it seems there are a lot of patches so it 
might take a while. I don't expect that you can get it merged before the 
next release. Some of the patches may need several versions or alternative 
approaches until they can be merged. For example I expect problems with 
allowing ',' in device names as this is something that was removed before 
to avoid the need of quoting or something like that. But I'm not in a 
hurry as I don't have much free time for it anyway so only come back to 
this time to time and it's far from anything useful yet.

>>
>> Best regards,
>> Bernhard
>>
>> P.S. The last commit teaches you how to start a firmware from SD card.

I did not try your version but looking at the patch looks like you have 
some sparse-mem region. (I've added similar one from board code, I did not 
know about this device.) Isn't that the l2cache as RAM or on-chip SRAM or 
whatever it's called? You seem to have some emulation of that l2cache in a 
previous patch so can't that be mapped there? Maybe we'll eventually need 
to implement reading the BOOT data from the beginning of the SD card or 
flash ROM which may have some initial register values that set things up 
that are currently hard coded.

Meanwhile I can cherry pick some patches from your tree and test them. 
Looks like you have some DDR3 support added, I haven't got to that yet.

For USB I had this for now:

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ee17acdb69..54890d25f3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -900,6 +900,29 @@ static void ppce500_power_off(void *opaque, int line, int on)
      }
  }

+static uint64_t usb_read(void *opaque, hwaddr addr, unsigned size)
+{
+    switch (addr) {
+    case 0:
+        return BIT(2) | BIT(17);
+    }
+    return 0;
+}
+
+static void usb_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+}
+
+static const MemoryRegionOps usb_ops = {
+    .read = usb_read,
+    .write = usb_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
  void ppce500_init(MachineState *machine)
  {
      MemoryRegion *address_space_mem = get_system_memory();
@@ -1118,6 +1141,19 @@ void ppce500_init(MachineState *machine)
                                      sysbus_mmio_get_region(s, 0));
      }

+    /* USB */
+    dev = qdev_new("tegra2-ehci-usb");
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(s, &error_fatal);
+    sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, 12));
+    memory_region_add_subregion(ccsr_addr_space, 0x22000,
+                                sysbus_mmio_get_region(s, 0));
+    {
+        MemoryRegion *mr =  g_new(MemoryRegion, 1);
+        memory_region_init_io(mr, OBJECT(dev), &usb_ops, s, "fsl-ehci", 4);
+        memory_region_add_subregion(ccsr_addr_space, 0x22500, mr);
+    }
+
      /* General Utility device */
      dev = qdev_new("mpc8544-guts");
      s = SYS_BUS_DEVICE(dev);

which is reusing a sufficiently similar existing device just to have 
minimal changes. This isn't the right way but since most of these just 
differ in the reg offsets I wonder if we could turn these offsets into 
properties so we don't need to add a new subclass for every device. I 
think subclasses came from the pci version where the PCI IDs are different 
and maybe sysbus was modelled after that but we only need subclasses where 
additional registers are needed (which may be the case for this fsl device 
so this property idea is just unrelated clean up).

Regards,
BALATON Zoltan