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

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

[BALATON Zoltan]

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.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/e500-ccsr.h |  4 +++
 hw/ppc/e500.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
index 249c17be3b..cfbf96e181 100644
--- a/hw/ppc/e500-ccsr.h
+++ b/hw/ppc/e500-ccsr.h
@@ -4,12 +4,16 @@
 #include "hw/sysbus.h"
 #include "qom/object.h"
 
+#define NR_LAWS 12
+
 struct PPCE500CCSRState {
     /*< private >*/
     SysBusDevice parent;
     /*< public >*/
 
     MemoryRegion ccsr_space;
+
+    uint32_t law_regs[NR_LAWS * 2];
 };
 
 #define TYPE_CCSR "e500-ccsr"
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 64f8c766b4..376cb4cb37 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -43,6 +43,7 @@
 #include "qemu/host-utils.h"
 #include "qemu/option.h"
 #include "hw/pci-host/ppce500.h"
+#include "qemu/log.h"
 #include "qemu/error-report.h"
 #include "hw/platform-bus.h"
 #include "hw/net/fsl_etsec/etsec.h"
@@ -1331,11 +1332,83 @@ void ppce500_init(MachineState *machine)
     boot_info->dt_size = dt_size;
 }
 
+static int law_idx(hwaddr addr)
+{
+    int idx;
+
+    addr -= 0xc08;
+    idx = 2 * ((addr >> 5) & 0xf);
+    if (addr & 8) {
+        idx++;
+    }
+    assert(idx < 2 * NR_LAWS);
+    return idx;
+}
+
+static uint64_t law_read(void *opaque, hwaddr addr, unsigned size)
+{
+    PPCE500CCSRState *s = opaque;
+    uint64_t val = 0;
+
+    switch (addr) {
+    case 0:
+        val = s->ccsr_space.addr >> 12;
+        break;
+    case 0xc08 ... 0xd70:
+        val = s->law_regs[law_idx(addr)];
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register read"
+                      "0x%" HWADDR_PRIx "\n", addr);
+    }
+    return val;
+}
+
+static void law_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    PPCE500CCSRState *s = opaque;
+
+    switch (addr) {
+    case 0:
+        val &= 0xffff00;
+        memory_region_set_address(&s->ccsr_space, val << 12);
+        break;
+    case 0xc08 ... 0xd70:
+    {
+        int idx = law_idx(addr);
+
+        qemu_log_mask(LOG_UNIMP, "Unimplemented local access register write"
+                      "0x%" HWADDR_PRIx " <- 0x%" PRIx64 "\n", addr, val);
+        val &= (idx & 1) ? 0x80f0003f : 0xffffff;
+        s->law_regs[idx] = val;
+        break;
+    }
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register write"
+                      "0x%" HWADDR_PRIx "\n", addr);
+    }
+}
+
+static const MemoryRegionOps law_ops = {
+    .read = law_read,
+    .write = law_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
 static void e500_ccsr_initfn(Object *obj)
 {
-    PPCE500CCSRState *ccsr = CCSR(obj);
-    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
-                       MPC8544_CCSRBAR_SIZE);
+    PPCE500CCSRState *s = CCSR(obj);
+    MemoryRegion *mr;
+
+    memory_region_init(&s->ccsr_space, obj, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
+
+    mr = g_new(MemoryRegion, 1);
+    memory_region_init_io(mr, obj, &law_ops, s, "local-access", 4096);
+    memory_region_add_subregion(&s->ccsr_space, 0, mr);
 }
 
 static const TypeInfo e500_ccsr_info = {
-- 
2.30.9

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

[BALATON Zoltan]

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?

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/e500-ccsr.h |  4 +++
> hw/ppc/e500.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
> index 249c17be3b..cfbf96e181 100644
> --- a/hw/ppc/e500-ccsr.h
> +++ b/hw/ppc/e500-ccsr.h
> @@ -4,12 +4,16 @@
> #include "hw/sysbus.h"
> #include "qom/object.h"
>
> +#define NR_LAWS 12
> +
> struct PPCE500CCSRState {
>     /*< private >*/
>     SysBusDevice parent;
>     /*< public >*/
>
>     MemoryRegion ccsr_space;
> +
> +    uint32_t law_regs[NR_LAWS * 2];
> };
>
> #define TYPE_CCSR "e500-ccsr"
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 64f8c766b4..376cb4cb37 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -43,6 +43,7 @@
> #include "qemu/host-utils.h"
> #include "qemu/option.h"
> #include "hw/pci-host/ppce500.h"
> +#include "qemu/log.h"
> #include "qemu/error-report.h"
> #include "hw/platform-bus.h"
> #include "hw/net/fsl_etsec/etsec.h"
> @@ -1331,11 +1332,83 @@ void ppce500_init(MachineState *machine)
>     boot_info->dt_size = dt_size;
> }
>
> +static int law_idx(hwaddr addr)
> +{
> +    int idx;
> +
> +    addr -= 0xc08;
> +    idx = 2 * ((addr >> 5) & 0xf);
> +    if (addr & 8) {
> +        idx++;
> +    }
> +    assert(idx < 2 * NR_LAWS);
> +    return idx;
> +}
> +
> +static uint64_t law_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PPCE500CCSRState *s = opaque;
> +    uint64_t val = 0;
> +
> +    switch (addr) {
> +    case 0:
> +        val = s->ccsr_space.addr >> 12;
> +        break;
> +    case 0xc08 ... 0xd70:
> +        val = s->law_regs[law_idx(addr)];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register read"
> +                      "0x%" HWADDR_PRIx "\n", addr);
> +    }
> +    return val;
> +}
> +
> +static void law_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PPCE500CCSRState *s = opaque;
> +
> +    switch (addr) {
> +    case 0:
> +        val &= 0xffff00;
> +        memory_region_set_address(&s->ccsr_space, val << 12);
> +        break;
> +    case 0xc08 ... 0xd70:
> +    {
> +        int idx = law_idx(addr);
> +
> +        qemu_log_mask(LOG_UNIMP, "Unimplemented local access register write"
> +                      "0x%" HWADDR_PRIx " <- 0x%" PRIx64 "\n", addr, val);
> +        val &= (idx & 1) ? 0x80f0003f : 0xffffff;
> +        s->law_regs[idx] = val;
> +        break;
> +    }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register write"
> +                      "0x%" HWADDR_PRIx "\n", addr);
> +    }
> +}
> +
> +static const MemoryRegionOps law_ops = {
> +    .read = law_read,
> +    .write = law_write,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> static void e500_ccsr_initfn(Object *obj)
> {
> -    PPCE500CCSRState *ccsr = CCSR(obj);
> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
> -                       MPC8544_CCSRBAR_SIZE);
> +    PPCE500CCSRState *s = CCSR(obj);
> +    MemoryRegion *mr;
> +
> +    memory_region_init(&s->ccsr_space, obj, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
> +
> +    mr = g_new(MemoryRegion, 1);
> +    memory_region_init_io(mr, obj, &law_ops, s, "local-access", 4096);
> +    memory_region_add_subregion(&s->ccsr_space, 0, mr);
> }
>
> static const TypeInfo e500_ccsr_info = {
>

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

[Bernhard Beschow]

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'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?

Best regards,
Bernhard

P.S. The last commit teaches you how to start a firmware from SD card.

>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/ppc/e500-ccsr.h |  4 +++
>> hw/ppc/e500.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 80 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
>> index 249c17be3b..cfbf96e181 100644
>> --- a/hw/ppc/e500-ccsr.h
>> +++ b/hw/ppc/e500-ccsr.h
>> @@ -4,12 +4,16 @@
>> #include "hw/sysbus.h"
>> #include "qom/object.h"
>> 
>> +#define NR_LAWS 12
>> +
>> struct PPCE500CCSRState {
>>     /*< private >*/
>>     SysBusDevice parent;
>>     /*< public >*/
>> 
>>     MemoryRegion ccsr_space;
>> +
>> +    uint32_t law_regs[NR_LAWS * 2];
>> };
>> 
>> #define TYPE_CCSR "e500-ccsr"
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 64f8c766b4..376cb4cb37 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -43,6 +43,7 @@
>> #include "qemu/host-utils.h"
>> #include "qemu/option.h"
>> #include "hw/pci-host/ppce500.h"
>> +#include "qemu/log.h"
>> #include "qemu/error-report.h"
>> #include "hw/platform-bus.h"
>> #include "hw/net/fsl_etsec/etsec.h"
>> @@ -1331,11 +1332,83 @@ void ppce500_init(MachineState *machine)
>>     boot_info->dt_size = dt_size;
>> }
>> 
>> +static int law_idx(hwaddr addr)
>> +{
>> +    int idx;
>> +
>> +    addr -= 0xc08;
>> +    idx = 2 * ((addr >> 5) & 0xf);
>> +    if (addr & 8) {
>> +        idx++;
>> +    }
>> +    assert(idx < 2 * NR_LAWS);
>> +    return idx;
>> +}
>> +
>> +static uint64_t law_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    PPCE500CCSRState *s = opaque;
>> +    uint64_t val = 0;
>> +
>> +    switch (addr) {
>> +    case 0:
>> +        val = s->ccsr_space.addr >> 12;
>> +        break;
>> +    case 0xc08 ... 0xd70:
>> +        val = s->law_regs[law_idx(addr)];
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register read"
>> +                      "0x%" HWADDR_PRIx "\n", addr);
>> +    }
>> +    return val;
>> +}
>> +
>> +static void law_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> +{
>> +    PPCE500CCSRState *s = opaque;
>> +
>> +    switch (addr) {
>> +    case 0:
>> +        val &= 0xffff00;
>> +        memory_region_set_address(&s->ccsr_space, val << 12);
>> +        break;
>> +    case 0xc08 ... 0xd70:
>> +    {
>> +        int idx = law_idx(addr);
>> +
>> +        qemu_log_mask(LOG_UNIMP, "Unimplemented local access register write"
>> +                      "0x%" HWADDR_PRIx " <- 0x%" PRIx64 "\n", addr, val);
>> +        val &= (idx & 1) ? 0x80f0003f : 0xffffff;
>> +        s->law_regs[idx] = val;
>> +        break;
>> +    }
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register write"
>> +                      "0x%" HWADDR_PRIx "\n", addr);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps law_ops = {
>> +    .read = law_read,
>> +    .write = law_write,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>> +};
>> +
>> static void e500_ccsr_initfn(Object *obj)
>> {
>> -    PPCE500CCSRState *ccsr = CCSR(obj);
>> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>> -                       MPC8544_CCSRBAR_SIZE);
>> +    PPCE500CCSRState *s = CCSR(obj);
>> +    MemoryRegion *mr;
>> +
>> +    memory_region_init(&s->ccsr_space, obj, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
>> +
>> +    mr = g_new(MemoryRegion, 1);
>> +    memory_region_init_io(mr, obj, &law_ops, s, "local-access", 4096);
>> +    memory_region_add_subregion(&s->ccsr_space, 0, mr);
>> }
>> 
>> static const TypeInfo e500_ccsr_info = {
>> 

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

[Bernhard Beschow]

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'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?
>
>Best regards,
>Bernhard
>
>P.S. The last commit teaches you how to start a firmware from SD card.

Forgot to mention the link to the branch. Here it is: https://github.com/shentok/qemu/tree/e500-fdt

Best regards,
Bernhard

>
>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/ppc/e500-ccsr.h |  4 +++
>>> hw/ppc/e500.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++--
>>> 2 files changed, 80 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
>>> index 249c17be3b..cfbf96e181 100644
>>> --- a/hw/ppc/e500-ccsr.h
>>> +++ b/hw/ppc/e500-ccsr.h
>>> @@ -4,12 +4,16 @@
>>> #include "hw/sysbus.h"
>>> #include "qom/object.h"
>>> 
>>> +#define NR_LAWS 12
>>> +
>>> struct PPCE500CCSRState {
>>>     /*< private >*/
>>>     SysBusDevice parent;
>>>     /*< public >*/
>>> 
>>>     MemoryRegion ccsr_space;
>>> +
>>> +    uint32_t law_regs[NR_LAWS * 2];
>>> };
>>> 
>>> #define TYPE_CCSR "e500-ccsr"
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index 64f8c766b4..376cb4cb37 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -43,6 +43,7 @@
>>> #include "qemu/host-utils.h"
>>> #include "qemu/option.h"
>>> #include "hw/pci-host/ppce500.h"
>>> +#include "qemu/log.h"
>>> #include "qemu/error-report.h"
>>> #include "hw/platform-bus.h"
>>> #include "hw/net/fsl_etsec/etsec.h"
>>> @@ -1331,11 +1332,83 @@ void ppce500_init(MachineState *machine)
>>>     boot_info->dt_size = dt_size;
>>> }
>>> 
>>> +static int law_idx(hwaddr addr)
>>> +{
>>> +    int idx;
>>> +
>>> +    addr -= 0xc08;
>>> +    idx = 2 * ((addr >> 5) & 0xf);
>>> +    if (addr & 8) {
>>> +        idx++;
>>> +    }
>>> +    assert(idx < 2 * NR_LAWS);
>>> +    return idx;
>>> +}
>>> +
>>> +static uint64_t law_read(void *opaque, hwaddr addr, unsigned size)
>>> +{
>>> +    PPCE500CCSRState *s = opaque;
>>> +    uint64_t val = 0;
>>> +
>>> +    switch (addr) {
>>> +    case 0:
>>> +        val = s->ccsr_space.addr >> 12;
>>> +        break;
>>> +    case 0xc08 ... 0xd70:
>>> +        val = s->law_regs[law_idx(addr)];
>>> +        break;
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register read"
>>> +                      "0x%" HWADDR_PRIx "\n", addr);
>>> +    }
>>> +    return val;
>>> +}
>>> +
>>> +static void law_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>> +{
>>> +    PPCE500CCSRState *s = opaque;
>>> +
>>> +    switch (addr) {
>>> +    case 0:
>>> +        val &= 0xffff00;
>>> +        memory_region_set_address(&s->ccsr_space, val << 12);
>>> +        break;
>>> +    case 0xc08 ... 0xd70:
>>> +    {
>>> +        int idx = law_idx(addr);
>>> +
>>> +        qemu_log_mask(LOG_UNIMP, "Unimplemented local access register write"
>>> +                      "0x%" HWADDR_PRIx " <- 0x%" PRIx64 "\n", addr, val);
>>> +        val &= (idx & 1) ? 0x80f0003f : 0xffffff;
>>> +        s->law_regs[idx] = val;
>>> +        break;
>>> +    }
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register write"
>>> +                      "0x%" HWADDR_PRIx "\n", addr);
>>> +    }
>>> +}
>>> +
>>> +static const MemoryRegionOps law_ops = {
>>> +    .read = law_read,
>>> +    .write = law_write,
>>> +    .endianness = DEVICE_BIG_ENDIAN,
>>> +    .valid = {
>>> +        .min_access_size = 4,
>>> +        .max_access_size = 4,
>>> +    },
>>> +};
>>> +
>>> static void e500_ccsr_initfn(Object *obj)
>>> {
>>> -    PPCE500CCSRState *ccsr = CCSR(obj);
>>> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>>> -                       MPC8544_CCSRBAR_SIZE);
>>> +    PPCE500CCSRState *s = CCSR(obj);
>>> +    MemoryRegion *mr;
>>> +
>>> +    memory_region_init(&s->ccsr_space, obj, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
>>> +
>>> +    mr = g_new(MemoryRegion, 1);
>>> +    memory_region_init_io(mr, obj, &law_ops, s, "local-access", 4096);
>>> +    memory_region_add_subregion(&s->ccsr_space, 0, mr);
>>> }
>>> 
>>> static const TypeInfo e500_ccsr_info = {
>>> 

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

[BALATON Zoltan]

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

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

[Bernhard Beschow]

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

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

[BALATON Zoltan]

On Thu, 6 Feb 2025, Bernhard Beschow wrote:
> 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.

Is that an alternative to commenting out page_aligned = true? (Previously 
it also needed some tweak on when to set DMA bit but after your fix in 
commit 5df50b8e973 in master resolved it that's no longer needed. Now only 
the interrupt control reset values and commenting page_aligned = true was 
needed to get it work.)

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

I've tested the firmware image available from this page:
http://eliyahu.org/tabor/setup.html
There's also a technical manual there that has info on the content of the 
SD card image.

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

There are two U-Boot binaries on the card for some reason (I think maybe 
the first one runs from cache as RAM and sets up the memory controller), 
then the first one loads some env variables and then the second U-Boot 
which then loads the bootloader. If you change one byte in the environment 
on the SD card it breaks the checksum and then it does not load the 
bootloader but gives a prompt. You can then look around in U-Boot. 
Alternatively you can extract the U-Boot binaries and convert to elf to 
load it with -bios then you can skip the SD card. Maybe depending on some 
env settings I haven't identified yet, the bootloader either says no 
bootable devices found or crashes when calling back into U-Boot which may 
be something similar that I had to fix for sam460ex here: 
https://gitlab.com/qemu-project/u-boot-sam460ex/-/commit/f6402c160f781145206b2dc0eb4d50738d0531d4/ 
but I don't have the Tabor U-Boot sources yet to check. Presumably it 
works on real machine so maybe it checks for SATA or other devices which 
aren't emulated so it may get an unexpected NULL value. I've tried adding 
dummy SATA emulation that shows an empty bus but it did not fix that. It 
could also be something completely different. With the USB patch, it at 
least finds USB storage devices after usb start in U-Boot but that's all I 
could test. I should find some Linux boot media known to work on real 
machine to test further but haven't had time for 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.
>
> 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

These existing machines set up values in PPCE500MachineClass in their init 
methods that the e500.c uses to change its behaviour to match the machine 
so to continue adding another board in the classic way I'd continue like 
that. I've added another similar board file like those machines setting 
the values matching P1022. For the additional devices in e500.c I've just 
patched them in for experimenting but these could be optionally created 
based on new values in the MachineClass, like has_2nd_i2c or similar to 
not change existing machines. I would not go into more elaborate solutions 
if your fdt based machine creation replaces this eventually.

> Buildroot recipes for the machines, both 32 and 64 bit, which might be 
> nice to keep working -- ideas welcome. Once the p10xx SoC is

I think the e500 machine was originally made for running with KVM and not 
for TCG emulation but it fell out of maintenance for a while and maybe 
Linux dropped some of the support by now so don't know if that still 
works. But it seems to still work for booting Linux in emulation so that 
should definitely be kept working.

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

This is new addition from today. I still don't get why you need sparse-mem 
when you also have a separate patch for l2cache regs which could have a 
memory region itself for this but for now I'm OK with adding this region 
from the tabor board code on my branch for experimenting. I'm a long way 
from this to work or being mergeable so your RFC might be ready and merged 
before that and then I don't need most of my changes, so I try to keep 
them minimal and have no intent trying to clean up the existing machines 
or SoC emulation.

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

Yes, the CHIPIDEA maybe is a better idea. I've only picked one that had 
the regs at the right place for a start and it seemed to work enough. But 
you likely have a more complete implementation so if you submit that 
eventually this will also be resolved for me so I can drop the above 
change and use yours instead. I just added it above because you seem to 
not have BIT(2) but I don't remember what that was and if it's really 
needed. Otherwise it looked you have the same.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

On Sun, 2 Feb 2025, BALATON Zoltan wrote:
> On Sat, 1 Feb 2025, Bernhard Beschow wrote:
>>> 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.

To solve the problem of needing reintroducing ',' in type names and adding 
a new subclass for every compatible device variant (which might also be 
wasteful if this creates a new type struct for these) maybe you'd have to 
attach a string array to classes or type info instead with all the 
compatible names and then look for the type to instantiate based on that. 
I don't know what the best way would be for that, adding a class property 
or adding this array to type? And maybe you need a new funcion to find the 
type instead of qdev_try_new().

I don't know how this works but the types seem to be stored in a hash 
table so instead of an array of compatible names maybe all that's needed 
is to allow adding alternative names for the same type in this type hash 
table or add another hash table for those alternative names then types 
could be looked up the same way from that by the compatible names without 
needing to subclass them multiple times. If these are separated hash 
tables then ',' may be allowed in the compatible names table or if it's 
the same types table then maybe the names can be encoded in some way to 
separate them from normal type names and avoid the need to reintroduce 
','. These are just vague ideas but maybe gives you some inspiration for 
which way to go to get closer to be able to upstream it.

Regards,
BALATON Zoltan

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

[Bernhard Beschow]

Am 7. Februar 2025 01:12:38 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Thu, 6 Feb 2025, Bernhard Beschow wrote:
>> 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.
>
>Is that an alternative to commenting out page_aligned = true?

Well, it's not needed with the patch applied. The patch ensures that all data gets loaded: <https://lore.kernel.org/qemu-devel/D62F06C8-5247-4FBC-82A9-9127352B30A6@gmail.com/>

>(Previously it also needed some tweak on when to set DMA bit but after your fix in commit 5df50b8e973 in master resolved it that's no longer needed. Now only the interrupt control reset values and commenting page_aligned = true was needed to get it work.)
>
>>> 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?
>
>I've tested the firmware image available from this page:
>http://eliyahu.org/tabor/setup.html
>There's also a technical manual there that has info on the content of the SD card image.

I got both from there as well.

>
>> 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...
>
>There are two U-Boot binaries on the card for some reason (I think maybe the first one runs from cache as RAM and sets up the memory controller), then the first one loads some env variables and then the second U-Boot which then loads the bootloader.

Yeah, that's the SPL and U-Boot proper. The first one sets up RAM based on DDR3 data, copies U-Boot proper there (currently broken in QEMU, see above), and passes control to it.

>If you change one byte in the environment on the SD card it breaks the checksum and then it does not load the bootloader but gives a prompt. You can then look around in U-Boot. Alternatively you can extract the U-Boot binaries and convert to elf to load it with -bios then you can skip the SD card. Maybe depending on some env settings I haven't identified yet, the bootloader either says no bootable devices found or crashes when calling back into U-Boot which may be something similar that I had to fix for sam460ex here: https://gitlab.com/qemu-project/u-boot-sam460ex/-/commit/f6402c160f781145206b2dc0eb4d50738d0531d4/ but I don't have the Tabor U-Boot sources yet to check. Presumably it works on real machine so maybe it checks for SATA or other devices which aren't emulated so it may get an unexpected NULL value. I've tried adding dummy SATA emulation that shows an empty bus but it did not fix that. It could also be something completely different. With the USB patch, it at least finds USB storage devices after usb start in U-Boot but that's all I could test. I should find some Linux boot media known to work on real machine to test further but haven't had time for it.

Considering the technical manual it may be possible to compile U-Boot oneself and replace components of the firmware image. That way, one could track down why no bootable devices are found, i.e. check whether and which bootmenu_x variables are populated. That's not on my todo list though.

>
>>>>> 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
>
>These existing machines set up values in PPCE500MachineClass in their init methods that the e500.c uses to change its behaviour to match the machine so to continue adding another board in the classic way I'd continue like that. I've added another similar board file like those machines setting the values matching P1022. For the additional devices in e500.c I've just patched them in for experimenting but these could be optionally created based on new values in the MachineClass, like has_2nd_i2c or similar to not change existing machines.

The challenge is that different variants of the SoC have the same IP cores mapped at different offsets with possibly (haven't checked) IRQs. These need to be considered when generating the DTB. To avoid dealing with this question -- and at the same time explore data-driven machine creation -- I reversed the process and generated the machine from the DTB. But this question needs to be answered when implementing a P1022.

>I would not go into more elaborate solutions if your fdt based machine creation replaces this eventually.

As said before, I'd send an RFC for discussion on how to proceed. 

>
>> Buildroot recipes for the machines, both 32 and 64 bit, which might be nice to keep working -- ideas welcome. Once the p10xx SoC is
>
>I think the e500 machine was originally made for running with KVM and not for TCG emulation but it fell out of maintenance for a while and maybe Linux dropped some of the support by now so don't know if that still works. But it seems to still work for booting Linux in emulation so that should definitely be kept working.

I haven't tried running on KVM since I don't have access to a PPC machine. Judging from the code both methods should work alike.
Buildroot still works because it builds an old toolchain, kernel, etc.

>
>> 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.
>
>This is new addition from today. I still don't get why you need sparse-mem when you also have a separate patch for l2cache regs which could have a memory region itself for this but for now I'm OK with adding this region from the tabor board code on my branch for experimenting.

The stack location is specific to U-Boot, not the chip. It resides in cache (used as RAM) rather than in SRAM. Modeling a cache with cache lines etc. was a rabbit hole I didn't want to get into. Hence I extended sparse-mem for this stack region to be user-creatable. When implementing a Tabor machine this stack region could be hardcoded there.

Best regards,
Bernhard

>I'm a long way from this to work or being mergeable so your RFC might be ready and merged before that and then I don't need most of my changes, so I try to keep them minimal and have no intent trying to clean up the existing machines or SoC emulation.
>
>>> 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.
>
>Yes, the CHIPIDEA maybe is a better idea. I've only picked one that had the regs at the right place for a start and it seemed to work enough. But you likely have a more complete implementation so if you submit that eventually this will also be resolved for me so I can drop the above change and use yours instead. I just added it above because you seem to not have BIT(2) but I don't remember what that was and if it's really needed. Otherwise it looked you have the same.
>
>Regards,
>BALATON Zoltan

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

[BALATON Zoltan]

On Wed, 12 Feb 2025, Bernhard Beschow wrote:
> Am 7. Februar 2025 01:12:38 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Thu, 6 Feb 2025, Bernhard Beschow wrote:
>>> Am 2. Februar 2025 01:25:22 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>
>>> I had to apply 
>>> <https://github.com/shentok/qemu/commit/56550cbb2805be2913ad9c12d6d9c6a9a3bf6f2c> 
>>> to have the SPL load the whole U-Boot proper.
>>
>> Is that an alternative to commenting out page_aligned = true?
>
> Well, it's not needed with the patch applied. The patch ensures that all 
> data gets loaded: 
> <https://lore.kernel.org/qemu-devel/D62F06C8-5247-4FBC-82A9-9127352B30A6@gmail.com/>

I think the block layer has a solution for such long running operations 
and uses coroutines for that but I don't know how that works and I could 
not find useful documentation on it. But I don't understand the problem 
either so if you've solved it and submit a patch that can be merged that's 
good enough for me.

>> There are two U-Boot binaries on the card for some reason (I think 
>> maybe the first one runs from cache as RAM and sets up the memory 
>> controller), then the first one loads some env variables and then the 
>> second U-Boot which then loads the bootloader.
>
> Yeah, that's the SPL and U-Boot proper. The first one sets up RAM based 
> on DDR3 data, copies U-Boot proper there (currently broken in QEMU, see 
> above), and passes control to it.

U-Boot proper runs for me too but it may have a bug in the Tabor specific 
patch (maybe revealed by missing emulation) that prevents the bootloader 
to find a device but I can run until U-Boot and enter commands to it, it 
only stops in the bootloader when that calls back to U-Boot. If you 
extract U-Boot proper from the SD card image, convert it to elf and run it 
with -bios you'd get a prompt so it works. That's what I do now for 
experimenting but I'd like it to boot from the SD card the same way as 
real machine does eventually.

> Considering the technical manual it may be possible to compile U-Boot 
> oneself and replace components of the firmware image. That way, one 
> could track down why no bootable devices are found, i.e. check whether 
> and which bootmenu_x variables are populated. That's not on my todo list 
> though.

First of all one would need the source for that which should be available 
because of GPL but it's not, at least I could not find it yet. Eventually 
I'll get it and may look at this but I'd also like to run binary known to 
work on real machine to make sure emulation is correct.

>> These existing machines set up values in PPCE500MachineClass in their 
>> init methods that the e500.c uses to change its behaviour to match the 
>> machine so to continue adding another board in the classic way I'd 
>> continue like that. I've added another similar board file like those 
>> machines setting the values matching P1022. For the additional devices 
>> in e500.c I've just patched them in for experimenting but these could 
>> be optionally created based on new values in the MachineClass, like 
>> has_2nd_i2c or similar to not change existing machines.
>
> The challenge is that different variants of the SoC have the same IP 
> cores mapped at different offsets with possibly (haven't checked) IRQs. 
> These need to be considered when generating the DTB. To avoid dealing 
> with this question -- and at the same time explore data-driven machine 
> creation -- I reversed the process and generated the machine from the 
> DTB. But this question needs to be answered when implementing a P1022.

Yes, your DTB based board code is a nice way to create different machines 
as the DTB already describes these offsets and irq connections and your 
code seems to be quite simple so I think it's a good idea that's worth 
pursuing, that could enhance the ppce500 machine and make it more generic. 
It could then also drop the separate mpc85xx machine and put all of these 
under one ppce500 machine with passing the right dtb to create the 
different machines. As long as these are similar enough and only differ in 
the devices they have, this could emulate a lot of these SoCs with the 
same code.

>> I would not go into more elaborate solutions if your fdt based machine 
>> creation replaces this eventually.
>
> As said before, I'd send an RFC for discussion on how to proceed.

As I wrote before I think one issue to solve as a next step would be avoid 
needing subclasses with comma in names for all compatible devices. I think 
allowing alternative names for the same type in the types hash table could 
be enough but to hide these from users and -device which can't take comma 
names, these might need to go in a separate hash table instead. This could 
work the same as register_type but would be something like 
register_compatible_name_for_type that adds only a name for an existing 
type so it does not need a subclass and duplicate class or type struct 
when all we need is a different name for the existing type. I don't know 
if it's best put in a new hash table or the same one that holds type 
names. E.g. machine names already have aliases for versioned machines for 
example. This could be similar for devices that could have alias names for 
compatible property in the DTB. Putting these in a separate hash table 
might be needed if adding them to the same type name would list them in 
-device help or having names with ',' there would cause a problem but it 
might even work without that.

These are only high level ideas and I don't know how to implement these 
but maybe you can do something with it. In any case, I'm looking forward 
for your RFC and try to help reviewing it but I don't have free time to 
contribute so I'll continue experimenting with the current code upstream 
that's good enough for what I do and it's faw away to try to upstream it 
so by then your changes could be merged. What I may contribute is new 
device emulation for missing parts. I have some dummy sata that does 
nothing but allows the Linux driver to pass detecting no devices, a half 
working DIU I made in the last few days that needs more work but I got 
some image from U-Boot on it and may look at the DMA controller in the 
future. Let me know if you're interested in these or have something for 
these or other parts I could use instead. I've tested your SPI flash 
implementation but it wasn't working with U-Boot for me and may look at 
your USB eventually.

>>> 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.
>>
>> This is new addition from today. I still don't get why you need 
>> sparse-mem when you also have a separate patch for l2cache regs which 
>> could have a memory region itself for this but for now I'm OK with 
>> adding this region from the tabor board code on my branch for 
>> experimenting.
>
> The stack location is specific to U-Boot, not the chip. It resides in 
> cache (used as RAM) rather than in SRAM. Modeling a cache with cache 
> lines etc. was a rabbit hole I didn't want to get into. Hence I extended 
> sparse-mem for this stack region to be user-creatable. When implementing 
> a Tabor machine this stack region could be hardcoded there.

I'm not sure I follow what you say but also not sure if you meant cache 
when writing stack in some places. Sure, the whole cache with all its 
details does not need to be modelled but what I meant is that if you 
emulate l2cache controller registers you could emulate the one that maps 
the cache at some address as sram then the BOOT table at the beginning of 
the SD card should write that to map it at the right place so if you also 
have code to parse that then no sparse-mem should be needed on the command 
line. Or is that some other register not part of l2cache? But this is 
something that can be sorted later so not a big deal, until then adding s 
memory region there either from board code or from command line will do. I 
think I get now that maybe you mean that command line is better because 
different boards have this at different location so you need to specify 
the address and that's easier with -device.

Regards,
BALATON Zoltan

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

[Bernhard Beschow]

Am 13. Februar 2025 00:13:24 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 12 Feb 2025, Bernhard Beschow wrote:
>> Am 7. Februar 2025 01:12:38 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Thu, 6 Feb 2025, Bernhard Beschow wrote:
>>>> Am 2. Februar 2025 01:25:22 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> 
>>>> I had to apply <https://github.com/shentok/qemu/commit/56550cbb2805be2913ad9c12d6d9c6a9a3bf6f2c> to have the SPL load the whole U-Boot proper.
>>> 
>>> Is that an alternative to commenting out page_aligned = true?
>> 
>> Well, it's not needed with the patch applied. The patch ensures that all data gets loaded: <https://lore.kernel.org/qemu-devel/D62F06C8-5247-4FBC-82A9-9127352B30A6@gmail.com/>
>
>I think the block layer has a solution for such long running operations and uses coroutines for that but I don't know how that works and I could not find useful documentation on it. But I don't understand the problem either so if you've solved it and submit a patch that can be merged that's good enough for me.
>
>>> There are two U-Boot binaries on the card for some reason (I think maybe the first one runs from cache as RAM and sets up the memory controller), then the first one loads some env variables and then the second U-Boot which then loads the bootloader.
>> 
>> Yeah, that's the SPL and U-Boot proper. The first one sets up RAM based on DDR3 data, copies U-Boot proper there (currently broken in QEMU, see above), and passes control to it.
>
>U-Boot proper runs for me too but it may have a bug in the Tabor specific patch (maybe revealed by missing emulation) that prevents the bootloader to find a device but I can run until U-Boot and enter commands to it, it only stops in the bootloader when that calls back to U-Boot. If you extract U-Boot proper from the SD card image, convert it to elf and run it with -bios you'd get a prompt so it works. That's what I do now for experimenting but I'd like it to boot from the SD card the same way as real machine does eventually.
>
>> Considering the technical manual it may be possible to compile U-Boot oneself and replace components of the firmware image. That way, one could track down why no bootable devices are found, i.e. check whether and which bootmenu_x variables are populated. That's not on my todo list though.
>
>First of all one would need the source for that which should be available because of GPL but it's not, at least I could not find it yet. Eventually I'll get it and may look at this but I'd also like to run binary known to work on real machine to make sure emulation is correct.
>
>>> These existing machines set up values in PPCE500MachineClass in their init methods that the e500.c uses to change its behaviour to match the machine so to continue adding another board in the classic way I'd continue like that. I've added another similar board file like those machines setting the values matching P1022. For the additional devices in e500.c I've just patched them in for experimenting but these could be optionally created based on new values in the MachineClass, like has_2nd_i2c or similar to not change existing machines.
>> 
>> The challenge is that different variants of the SoC have the same IP cores mapped at different offsets with possibly (haven't checked) IRQs. These need to be considered when generating the DTB. To avoid dealing with this question -- and at the same time explore data-driven machine creation -- I reversed the process and generated the machine from the DTB. But this question needs to be answered when implementing a P1022.
>
>Yes, your DTB based board code is a nice way to create different machines as the DTB already describes these offsets and irq connections and your code seems to be quite simple so I think it's a good idea that's worth pursuing, that could enhance the ppce500 machine and make it more generic. It could then also drop the separate mpc85xx machine and put all of these under one ppce500 machine with passing the right dtb to create the different machines. As long as these are similar enough and only differ in the devices they have, this could emulate a lot of these SoCs with the same code.

The existing machines can already be created from a DTB, see the pc-bios folder which contains the two respective .dts files.

>
>>> I would not go into more elaborate solutions if your fdt based machine creation replaces this eventually.
>> 
>> As said before, I'd send an RFC for discussion on how to proceed.
>
>As I wrote before I think one issue to solve as a next step would be avoid needing subclasses with comma in names for all compatible devices. I think allowing alternative names for the same type in the types hash table could be enough but to hide these from users and -device which can't take comma names, these might need to go in a separate hash table instead. This could work the same as register_type but would be something like register_compatible_name_for_type that adds only a name for an existing type so it does not need a subclass and duplicate class or type struct when all we need is a different name for the existing type. I don't know if it's best put in a new hash table or the same one that holds type names. E.g. machine names already have aliases for versioned machines for example. This could be similar for devices that could have alias names for compatible property in the DTB. Putting these in a separate hash table might be needed if adding them to the same type name would list them in -device help or having names with ',' there would cause a problem but it might even work without that.
>
>These are only high level ideas and I don't know how to implement these but maybe you can do something with it. In any case, I'm looking forward for your RFC and try to help reviewing it but I don't have free time to contribute so I'll continue experimenting with the current code upstream that's good enough for what I do and it's faw away to try to upstream it so by then your changes could be merged.

Thanks for your input, I'll look into it closer after my RFC. Right now I'm quite busy driving the i.MX 8M Plus machine forward, hence my delayed responses.

>What I may contribute is new device emulation for missing parts. I have some dummy sata that does nothing but allows the Linux driver to pass detecting no devices, a half working DIU I made in the last few days that needs more work but I got some image from U-Boot on it and may look at the DMA controller in the future. Let me know if you're interested in these or have something for these or other parts I could use instead. I've tested your SPI flash implementation but it wasn't working with U-Boot for me and may look at your USB eventually.

Your additions sound promising! And thanks for testing the devices.

>
>>>> 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.
>>> 
>>> This is new addition from today. I still don't get why you need sparse-mem when you also have a separate patch for l2cache regs which could have a memory region itself for this but for now I'm OK with adding this region from the tabor board code on my branch for experimenting.
>> 
>> The stack location is specific to U-Boot, not the chip. It resides in cache (used as RAM) rather than in SRAM. Modeling a cache with cache lines etc. was a rabbit hole I didn't want to get into. Hence I extended sparse-mem for this stack region to be user-creatable. When implementing a Tabor machine this stack region could be hardcoded there.
>
>I'm not sure I follow what you say but also not sure if you meant cache when writing stack in some places. Sure, the whole cache with all its details does not need to be modelled but what I meant is that if you emulate l2cache controller registers you could emulate the one that maps the cache at some address as sram then the BOOT table at the beginning of the SD card should write that to map it at the right place so if you also have code to parse that then no sparse-mem should be needed on the command line. Or is that some other register not part of l2cache? But this is something that can be sorted later so not a big deal, until then adding s memory region there either from board code or from command line will do. I think I get now that maybe you mean that command line is better because different boards have this at different location so you need to specify the address and that's easier with -device.

The l2cache controller manages 256kB of memory which is configured as cache by default. The Tabor BOOT table configures part of it as RAM which involves mapping just that part into the address space. The SPL gets copied into  into it and is executed from there. At runtime the SPL sets up a stack. This is another memory region disjoint from the SPL's. Rather than configuring the new region as RAM, you can think of this region to be marked just as cacheable in the MMU. Hence, when the SPL uses this region as stack, the data will be captured by the cache rather than ending up in the void. In QEMU, the l2cache implementation creates the SPL region and the stack region is modeled by the sparse-mem device in order to avoid implementing a cache.

HTH,
Bernhard

>
>Regards,
>BALATON Zoltan

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

[BALATON Zoltan]

On Thu, 20 Feb 2025, Bernhard Beschow wrote:
> Am 13. Februar 2025 00:13:24 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> Yes, your DTB based board code is a nice way to create different 
>> machines as the DTB already describes these offsets and irq connections 
>> and your code seems to be quite simple so I think it's a good idea 
>> that's worth pursuing, that could enhance the ppce500 machine and make 
>> it more generic. It could then also drop the separate mpc85xx machine 
>> and put all of these under one ppce500 machine with passing the right 
>> dtb to create the different machines. As long as these are similar 
>> enough and only differ in the devices they have, this could emulate a 
>> lot of these SoCs with the same code.
>
> The existing machines can already be created from a DTB, see the pc-bios 
> folder which contains the two respective .dts files.

That's why I said that once this is merged we could deprecate the existing 
machines in favour of ppc500 with the dts for these machines replacing 
them so we can get rid of board codes and can add new machines like p1022 
by adding the needed devices and the dts also under ppce500 which can 
become a generic machine supporting all of these. One could then use -M 
ppce500 -dtb mpc85xx.dtb or p1022.dtb or similar. This would work for all 
dtbs that we have devices for and maybe by adding unimplemented device for 
unknown ones would allow somewhat booting other machines as long as those 
devices are not use. Or if keeping a machine for the -M option for all of 
these is desired then we can think of adding some other kind of alias that 
can set the dtb for the ppce500 and call that. But in any case this would 
reduce the mpc85xx to a trivial wrapper of ppce500 that just sets the dtb 
for the machine.

> Thanks for your input, I'll look into it closer after my RFC. Right now 
> I'm quite busy driving the i.MX 8M Plus machine forward, hence my 
> delayed responses.
>
>> What I may contribute is new device emulation for missing parts. I have 
>> some dummy sata that does nothing but allows the Linux driver to pass 
>> detecting no devices, a half working DIU I made in the last few days 
>> that needs more work but I got some image from U-Boot on it and may 
>> look at the DMA controller in the future. Let me know if you're 
>> interested in these or have something for these or other parts I could 
>> use instead. I've tested your SPI flash implementation but it wasn't 
>> working with U-Boot for me and may look at your USB eventually.
>
> Your additions sound promising! And thanks for testing the devices.

I also got distracted with other machines so I've postponed this for now 
but want to get back to it eventually.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

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.

Bernhard,

If you have no alternative patch you plan to submit for next release 
should we merge this for now? This helps running u-boot binaries even if 
it's not enough alone but would be one patch less in my local tree. Or do 
you know about a problem with this patch why this should not be merged?

Regards,
BALATON Zoltan

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/e500-ccsr.h |  4 +++
> hw/ppc/e500.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
> index 249c17be3b..cfbf96e181 100644
> --- a/hw/ppc/e500-ccsr.h
> +++ b/hw/ppc/e500-ccsr.h
> @@ -4,12 +4,16 @@
> #include "hw/sysbus.h"
> #include "qom/object.h"
>
> +#define NR_LAWS 12
> +
> struct PPCE500CCSRState {
>     /*< private >*/
>     SysBusDevice parent;
>     /*< public >*/
>
>     MemoryRegion ccsr_space;
> +
> +    uint32_t law_regs[NR_LAWS * 2];
> };
>
> #define TYPE_CCSR "e500-ccsr"
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 64f8c766b4..376cb4cb37 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -43,6 +43,7 @@
> #include "qemu/host-utils.h"
> #include "qemu/option.h"
> #include "hw/pci-host/ppce500.h"
> +#include "qemu/log.h"
> #include "qemu/error-report.h"
> #include "hw/platform-bus.h"
> #include "hw/net/fsl_etsec/etsec.h"
> @@ -1331,11 +1332,83 @@ void ppce500_init(MachineState *machine)
>     boot_info->dt_size = dt_size;
> }
>
> +static int law_idx(hwaddr addr)
> +{
> +    int idx;
> +
> +    addr -= 0xc08;
> +    idx = 2 * ((addr >> 5) & 0xf);
> +    if (addr & 8) {
> +        idx++;
> +    }
> +    assert(idx < 2 * NR_LAWS);
> +    return idx;
> +}
> +
> +static uint64_t law_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PPCE500CCSRState *s = opaque;
> +    uint64_t val = 0;
> +
> +    switch (addr) {
> +    case 0:
> +        val = s->ccsr_space.addr >> 12;
> +        break;
> +    case 0xc08 ... 0xd70:
> +        val = s->law_regs[law_idx(addr)];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register read"
> +                      "0x%" HWADDR_PRIx "\n", addr);
> +    }
> +    return val;
> +}
> +
> +static void law_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    PPCE500CCSRState *s = opaque;
> +
> +    switch (addr) {
> +    case 0:
> +        val &= 0xffff00;
> +        memory_region_set_address(&s->ccsr_space, val << 12);
> +        break;
> +    case 0xc08 ... 0xd70:
> +    {
> +        int idx = law_idx(addr);
> +
> +        qemu_log_mask(LOG_UNIMP, "Unimplemented local access register write"
> +                      "0x%" HWADDR_PRIx " <- 0x%" PRIx64 "\n", addr, val);
> +        val &= (idx & 1) ? 0x80f0003f : 0xffffff;
> +        s->law_regs[idx] = val;
> +        break;
> +    }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register write"
> +                      "0x%" HWADDR_PRIx "\n", addr);
> +    }
> +}
> +
> +static const MemoryRegionOps law_ops = {
> +    .read = law_read,
> +    .write = law_write,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> static void e500_ccsr_initfn(Object *obj)
> {
> -    PPCE500CCSRState *ccsr = CCSR(obj);
> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
> -                       MPC8544_CCSRBAR_SIZE);
> +    PPCE500CCSRState *s = CCSR(obj);
> +    MemoryRegion *mr;
> +
> +    memory_region_init(&s->ccsr_space, obj, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
> +
> +    mr = g_new(MemoryRegion, 1);
> +    memory_region_init_io(mr, obj, &law_ops, s, "local-access", 4096);
> +    memory_region_add_subregion(&s->ccsr_space, 0, mr);
> }
>
> static const TypeInfo e500_ccsr_info = {
>

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

[Bernhard Beschow]

Am 1. März 2025 16:10:35 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.
>
>Bernhard,

Hi Zoltan,

>
>If you have no alternative patch you plan to submit for next release should we merge this for now? This helps running u-boot binaries even if it's not enough alone but would be one patch less in my local tree. Or do you know about a problem with this patch why this should not be merged?

QEMU sets a machine-specific CCSR base address (pmc->ccsrbar_base) which differs from the real chip's default. The default is checked by U-Boot which enters an infinite loop on mismatch: <https://source.denx.de/u-boot/u-boot/-/blob/v2024.07/arch/powerpc/cpu/mpc85xx/start.S#L614>. How does this work for you?

In addition, when moving the CCSR region, `env->mpic_iack` should be updated as well: <https://gitlab.com/qemu-project/qemu/-/blob/v9.2.2/hw/ppc/e500.c?ref_type=tags#L969>

I'm happy to submit an implementation on top of my e500 cleanup series <https://patchew.org/QEMU/20241103133412.73536-1-shentey@gmail.com/> which needs agreement on some open discussions.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/ppc/e500-ccsr.h |  4 +++
>> hw/ppc/e500.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 80 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
>> index 249c17be3b..cfbf96e181 100644
>> --- a/hw/ppc/e500-ccsr.h
>> +++ b/hw/ppc/e500-ccsr.h
>> @@ -4,12 +4,16 @@
>> #include "hw/sysbus.h"
>> #include "qom/object.h"
>> 
>> +#define NR_LAWS 12
>> +
>> struct PPCE500CCSRState {
>>     /*< private >*/
>>     SysBusDevice parent;
>>     /*< public >*/
>> 
>>     MemoryRegion ccsr_space;
>> +
>> +    uint32_t law_regs[NR_LAWS * 2];
>> };
>> 
>> #define TYPE_CCSR "e500-ccsr"
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 64f8c766b4..376cb4cb37 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -43,6 +43,7 @@
>> #include "qemu/host-utils.h"
>> #include "qemu/option.h"
>> #include "hw/pci-host/ppce500.h"
>> +#include "qemu/log.h"
>> #include "qemu/error-report.h"
>> #include "hw/platform-bus.h"
>> #include "hw/net/fsl_etsec/etsec.h"
>> @@ -1331,11 +1332,83 @@ void ppce500_init(MachineState *machine)
>>     boot_info->dt_size = dt_size;
>> }
>> 
>> +static int law_idx(hwaddr addr)
>> +{
>> +    int idx;
>> +
>> +    addr -= 0xc08;
>> +    idx = 2 * ((addr >> 5) & 0xf);
>> +    if (addr & 8) {
>> +        idx++;
>> +    }
>> +    assert(idx < 2 * NR_LAWS);
>> +    return idx;
>> +}
>> +
>> +static uint64_t law_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    PPCE500CCSRState *s = opaque;
>> +    uint64_t val = 0;
>> +
>> +    switch (addr) {
>> +    case 0:
>> +        val = s->ccsr_space.addr >> 12;
>> +        break;
>> +    case 0xc08 ... 0xd70:
>> +        val = s->law_regs[law_idx(addr)];
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register read"
>> +                      "0x%" HWADDR_PRIx "\n", addr);
>> +    }
>> +    return val;
>> +}
>> +
>> +static void law_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> +{
>> +    PPCE500CCSRState *s = opaque;
>> +
>> +    switch (addr) {
>> +    case 0:
>> +        val &= 0xffff00;
>> +        memory_region_set_address(&s->ccsr_space, val << 12);
>> +        break;
>> +    case 0xc08 ... 0xd70:
>> +    {
>> +        int idx = law_idx(addr);
>> +
>> +        qemu_log_mask(LOG_UNIMP, "Unimplemented local access register write"
>> +                      "0x%" HWADDR_PRIx " <- 0x%" PRIx64 "\n", addr, val);
>> +        val &= (idx & 1) ? 0x80f0003f : 0xffffff;
>> +        s->law_regs[idx] = val;
>> +        break;
>> +    }
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid local access register write"
>> +                      "0x%" HWADDR_PRIx "\n", addr);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps law_ops = {
>> +    .read = law_read,
>> +    .write = law_write,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>> +};
>> +
>> static void e500_ccsr_initfn(Object *obj)
>> {
>> -    PPCE500CCSRState *ccsr = CCSR(obj);
>> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>> -                       MPC8544_CCSRBAR_SIZE);
>> +    PPCE500CCSRState *s = CCSR(obj);
>> +    MemoryRegion *mr;
>> +
>> +    memory_region_init(&s->ccsr_space, obj, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
>> +
>> +    mr = g_new(MemoryRegion, 1);
>> +    memory_region_init_io(mr, obj, &law_ops, s, "local-access", 4096);
>> +    memory_region_add_subregion(&s->ccsr_space, 0, mr);
>> }
>> 
>> static const TypeInfo e500_ccsr_info = {
>> 

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

On Sun, 2 Mar 2025, Bernhard Beschow wrote:
> Am 1. März 2025 16:10:35 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.
>>
>> Bernhard,
>
> Hi Zoltan,
>
>>
>> If you have no alternative patch you plan to submit for next release 
>> should we merge this for now? This helps running u-boot binaries even 
>> if it's not enough alone but would be one patch less in my local tree. 
>> Or do you know about a problem with this patch why this should not be 
>> merged?
>
> QEMU sets a machine-specific CCSR base address (pmc->ccsrbar_base) which 
> differs from the real chip's default. The default is checked by U-Boot 
> which enters an infinite loop on mismatch: 
> <https://source.denx.de/u-boot/u-boot/-/blob/v2024.07/arch/powerpc/cpu/mpc85xx/start.S#L614>. 
> How does this work for you?

The U-Boot version I've tested (or something else, I don't remember) 
wanted to set the CCSRBAR to the value it expects which is different from 
where it looks for it at reset and that works with this patch. I don't 
know which code path that corresponds to in start.S.

> In addition, when moving the CCSR region, `env->mpic_iack` should be 
> updated as well: 
> <https://gitlab.com/qemu-project/qemu/-/blob/v9.2.2/hw/ppc/e500.c?ref_type=tags#L969>

I did not notice that. Maybe it's only relevant with KVM and I've only 
tested with TCG or could be I did not get to the part yet when this would 
cause problems. I don't see an easy way to update this as these are in 
separate places and also don't know how this mpic_iack is handled with 
multiple cores. So in case this patch breaks this then it's OK to drop it 
for now. I can carry it locally until fixed upstream.

> I'm happy to submit an implementation on top of my e500 cleanup series 
> <https://patchew.org/QEMU/20241103133412.73536-1-shentey@gmail.com/> 
> which needs agreement on some open discussions.

Some of that series was already merged. What are the outstanding patches 
and discussions? I don't remember seeing an updated version of that series 
with only the outstanding patches.

Regards,
BALATON Zoltan