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

[Bernhard Beschow <shentey@gmail.com>]

Am 2. Februar 2025 01:25:22 UTC schrieb 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.

I had to apply <https://github.com/shentok/qemu/commit/56550cbb2805be2913ad9c12d6d9c6a9a3bf6f2c> to have the SPL load the whole U-Boot proper.

>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.

Do you use the Tabor U-Boot or something else? How do you get to the command prompt? For me, the bootloader application started by Tabor U-Boot doesn't crash but then doesn't find bootable devices, not even with an emulated USB stick. Instead of dropping to the command prompt it only offers a restart to the firmware which then starts the bootloader application again...

>
>>> 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.

My branch is not a maintainer tree. I neither expect it to be mergeable like this, nor is it my goal. Instead, it's just an experiment to investigate data-driven machine creation which I'd like to share as an RFC with the community.

That said, one could probably turn that branch into a p10xx SoC implemented in the classic way. For this, one would need to decide on how to proceed with the mpc8544ds and e500plat machines. There are Buildroot recipes for the machines, both 32 and 64 bit, which might be nice to keep working -- ideas welcome. Once the p10xx SoC is implemented, a tabor machine could be added which uses it.

>
>>> 
>>> 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.

This is implemented on my branch. It pokes the L2 cache registers to configure some (but not all) SRAM to load the SPL to. This SPL uses cache as RAM which I'm emulating with a modified sparse-mem region device.

>
>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).

My implementation has similar usb_ops but is based on TYPE_CHIPIDEA which also has the "endpoints" registers covered. It is used by some i.MX machines and given that these and p1022 are NXP SoCs I wouldn't be surprised if they shared a relation in the real world.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan