[PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup

[PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup

[Bernhard Beschow]

Cover letter:
~~~~~~~~~~~~~

This series adds support for -pflash and direct SD card access to the
PPC e500 boards. The idea is to increase compatibility with "real" firmware
images where only the bare minimum of drivers is compiled in.

The series is structured as follows:

Patches 1-4 perform some general cleanup which paves the way for the rest of
the series.

Patch 5 adds -pflash handling where memory-mapped flash can be added on
user's behalf. That is, the flash memory region in the eLBC is only added if
the -pflash argument is supplied. Note that the cfi01 device model becomes
stricter in checking the size of the emulated flash space.

Patches 6 and 7 add a new device model - the Freescale eSDHC - to the e500
boards which was missing so far.

User documentation is also added as the new features become available.

Tesing done:
* `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
"console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive
if=pflash,file=rootfs.ext2,format=raw`
* `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
"console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive
id=mydrive,if=none,file=rootfs.ext2,format=raw`

The load was created using latest Buildroot with `make
qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type.
In both cases it was possible to log in and explore the root file system.

v4:
~~~
Zoltan:
- Don't suggest presence of qemu-system-ppc32 in documentation

Bin:
- New patch: docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)

Peter:
- Inline pflash_cfi01_register() rather than modify it (similar to v2)

v3:
~~~
Phil:
- Also add power-of-2 fix to pflash_cfi02
- Resolve cfi01-specific assertion in e500 code
- Resolve unused define in eSDHC device model
- Resolve redundant alignment checks in eSDHC device model

Bin:
- Add dedicated flash chapter to documentation

Bernhard:
- Use is_power_of_2() instead of ctpop64() for better readability
- Only instantiate eSDHC device model in ppce500 (not used in MPC8544DS)
- Rebase onto gitlab.com/danielhb/qemu/tree/ppc-next

v2:
~~~
Bin:
- Add source for MPC8544DS platform bus' memory map in commit message.
- Keep "ESDHC" in comment referring to Linux driver.
- Use "qemu-system-ppc{64|32} in documentation.
- Use g_autofree in device tree code.
- Remove unneeded device tree properties.
- Error out if pflash size doesn't fit into eLBC memory window.
- Remove unused ESDHC defines.
- Define macro ESDHC_WML for register offset with magic constant.
- Fix some whitespace issues when adding eSDHC device to e500.

Phil:
- Fix tense in commit message.

Bernhard Beschow (7):
  docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
  hw/block/pflash_cfi0{1,2}: Error out if device length isn't a power of
    two
  hw/sd/sdhci-internal: Unexport ESDHC defines
  hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
  hw/ppc/e500: Implement pflash handling
  hw/sd/sdhci: Implement Freescale eSDHC device model
  hw/ppc/e500: Add Freescale eSDHC to e500plat

 docs/system/ppc/ppce500.rst |  38 +++++++-
 hw/block/pflash_cfi01.c     |   8 +-
 hw/block/pflash_cfi02.c     |   5 +
 hw/ppc/Kconfig              |   2 +
 hw/ppc/e500.c               | 114 +++++++++++++++++++++-
 hw/ppc/e500.h               |   1 +
 hw/ppc/e500plat.c           |   1 +
 hw/sd/sdhci-internal.h      |  20 ----
 hw/sd/sdhci.c               | 183 +++++++++++++++++++++++++++++++-----
 include/hw/sd/sdhci.h       |   3 +
 10 files changed, 324 insertions(+), 51 deletions(-)

-- 
2.38.0

[PATCH v4 1/7] docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)

[Bernhard Beschow]

The documentation suggests that there is a qemu-system-ppc32 binary
while the 32 bit version is actually just named qemu-system-ppc. Settle
on qemu-system-ppc64 which also works for 32 bit machines and causes
less clutter in the documentation.

Found-by: BALATON Zoltan <balaton@eik.bme.hu>
Suggested-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 docs/system/ppc/ppce500.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
index ba6bcb7314..7b5eb3c4ee 100644
--- a/docs/system/ppc/ppce500.rst
+++ b/docs/system/ppc/ppce500.rst
@@ -113,7 +113,7 @@ To boot the 32-bit Linux kernel:
 
 .. code-block:: bash
 
-  $ qemu-system-ppc{64|32} -M ppce500 -cpu e500mc -smp 4 -m 2G \
+  $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
       -display none -serial stdio \
       -kernel vmlinux \
       -initrd /path/to/rootfs.cpio \
@@ -154,10 +154,10 @@ interface at PCI address 0.1.0, but we can switch that to an e1000 NIC by:
 
 .. code-block:: bash
 
-  $ qemu-system-ppc -M ppce500 -smp 4 -m 2G \
-                    -display none -serial stdio \
-                    -bios u-boot \
-                    -nic tap,ifname=tap0,script=no,downscript=no,model=e1000
+  $ qemu-system-ppc64 -M ppce500 -smp 4 -m 2G \
+                      -display none -serial stdio \
+                      -bios u-boot \
+                      -nic tap,ifname=tap0,script=no,downscript=no,model=e1000
 
 The QEMU ``ppce500`` machine can also dynamically instantiate an eTSEC device
 if “-device eTSEC” is given to QEMU:
-- 
2.38.0

[PATCH v4 2/7] hw/block/pflash_cfi0{1, 2}: Error out if device length isn't a power of two

[Bernhard Beschow]

According to the JEDEC standard the device length is communicated to an
OS as an exponent (power of two).

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/pflash_cfi01.c | 8 ++++++--
 hw/block/pflash_cfi02.c | 5 +++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 0cbc2fb4cb..9c235bf66e 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -690,7 +690,7 @@ static const MemoryRegionOps pflash_cfi01_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl)
+static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl, Error **errp)
 {
     uint64_t blocks_per_device, sector_len_per_device, device_len;
     int num_devices;
@@ -708,6 +708,10 @@ static void pflash_cfi01_fill_cfi_table(PFlashCFI01 *pfl)
         sector_len_per_device = pfl->sector_len / num_devices;
     }
     device_len = sector_len_per_device * blocks_per_device;
+    if (!is_power_of_2(device_len)) {
+        error_setg(errp, "Device size must be a power of two.");
+        return;
+    }
 
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
@@ -865,7 +869,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
      */
     pfl->cmd = 0x00;
     pfl->status = 0x80; /* WSM ready */
-    pflash_cfi01_fill_cfi_table(pfl);
+    pflash_cfi01_fill_cfi_table(pfl, errp);
 }
 
 static void pflash_cfi01_system_reset(DeviceState *dev)
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 2a99b286b0..ff2fe154c1 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -880,6 +880,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (!is_power_of_2(pfl->chip_len)) {
+        error_setg(errp, "Device size must be a power of two.");
+        return;
+    }
+
     memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
                                   &pflash_cfi02_ops, pfl, pfl->name,
                                   pfl->chip_len, errp);
-- 
2.38.0

[PATCH v4 3/7] hw/sd/sdhci-internal: Unexport ESDHC defines

[Bernhard Beschow]

These defines aren't used outside of sdhci.c, so can be defined there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci-internal.h | 20 --------------------
 hw/sd/sdhci.c          | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index e8c753d6d1..964570f8e8 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -288,26 +288,6 @@ enum {
 
 extern const VMStateDescription sdhci_vmstate;
 
-
-#define ESDHC_MIX_CTRL                  0x48
-
-#define ESDHC_VENDOR_SPEC               0xc0
-#define ESDHC_IMX_FRC_SDCLK_ON          (1 << 8)
-
-#define ESDHC_DLL_CTRL                  0x60
-
-#define ESDHC_TUNING_CTRL               0xcc
-#define ESDHC_TUNE_CTRL_STATUS          0x68
-#define ESDHC_WTMK_LVL                  0x44
-
-/* Undocumented register used by guests working around erratum ERR004536 */
-#define ESDHC_UNDOCUMENTED_REG27        0x6c
-
-#define ESDHC_CTRL_4BITBUS              (0x1 << 1)
-#define ESDHC_CTRL_8BITBUS              (0x2 << 1)
-
-#define ESDHC_PRNSTS_SDSTB              (1 << 3)
-
 /*
  * Default SD/MMC host controller features information, which will be
  * presented in CAPABILITIES register of generic SD host controller at reset.
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0e5e988927..6da5e2c781 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1577,6 +1577,25 @@ static const TypeInfo sdhci_bus_info = {
 
 /* --- qdev i.MX eSDHC --- */
 
+#define ESDHC_MIX_CTRL                  0x48
+
+#define ESDHC_VENDOR_SPEC               0xc0
+#define ESDHC_IMX_FRC_SDCLK_ON          (1 << 8)
+
+#define ESDHC_DLL_CTRL                  0x60
+
+#define ESDHC_TUNING_CTRL               0xcc
+#define ESDHC_TUNE_CTRL_STATUS          0x68
+#define ESDHC_WTMK_LVL                  0x44
+
+/* Undocumented register used by guests working around erratum ERR004536 */
+#define ESDHC_UNDOCUMENTED_REG27        0x6c
+
+#define ESDHC_CTRL_4BITBUS              (0x1 << 1)
+#define ESDHC_CTRL_8BITBUS              (0x2 << 1)
+
+#define ESDHC_PRNSTS_SDSTB              (1 << 3)
+
 static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
 {
     SDHCIState *s = SYSBUS_SDHCI(opaque);
-- 
2.38.0

[PATCH v4 4/7] hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*

[Bernhard Beschow]

The device model's functions start with "usdhc_", so rename the defines
accordingly for consistency.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/sd/sdhci.c | 66 +++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6da5e2c781..306070c872 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1577,24 +1577,24 @@ static const TypeInfo sdhci_bus_info = {
 
 /* --- qdev i.MX eSDHC --- */
 
-#define ESDHC_MIX_CTRL                  0x48
+#define USDHC_MIX_CTRL                  0x48
 
-#define ESDHC_VENDOR_SPEC               0xc0
-#define ESDHC_IMX_FRC_SDCLK_ON          (1 << 8)
+#define USDHC_VENDOR_SPEC               0xc0
+#define USDHC_IMX_FRC_SDCLK_ON          (1 << 8)
 
-#define ESDHC_DLL_CTRL                  0x60
+#define USDHC_DLL_CTRL                  0x60
 
-#define ESDHC_TUNING_CTRL               0xcc
-#define ESDHC_TUNE_CTRL_STATUS          0x68
-#define ESDHC_WTMK_LVL                  0x44
+#define USDHC_TUNING_CTRL               0xcc
+#define USDHC_TUNE_CTRL_STATUS          0x68
+#define USDHC_WTMK_LVL                  0x44
 
 /* Undocumented register used by guests working around erratum ERR004536 */
-#define ESDHC_UNDOCUMENTED_REG27        0x6c
+#define USDHC_UNDOCUMENTED_REG27        0x6c
 
-#define ESDHC_CTRL_4BITBUS              (0x1 << 1)
-#define ESDHC_CTRL_8BITBUS              (0x2 << 1)
+#define USDHC_CTRL_4BITBUS              (0x1 << 1)
+#define USDHC_CTRL_8BITBUS              (0x2 << 1)
 
-#define ESDHC_PRNSTS_SDSTB              (1 << 3)
+#define USDHC_PRNSTS_SDSTB              (1 << 3)
 
 static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
 {
@@ -1615,11 +1615,11 @@ static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
         hostctl1 = SDHC_DMA_TYPE(s->hostctl1) << (8 - 3);
 
         if (s->hostctl1 & SDHC_CTRL_8BITBUS) {
-            hostctl1 |= ESDHC_CTRL_8BITBUS;
+            hostctl1 |= USDHC_CTRL_8BITBUS;
         }
 
         if (s->hostctl1 & SDHC_CTRL_4BITBUS) {
-            hostctl1 |= ESDHC_CTRL_4BITBUS;
+            hostctl1 |= USDHC_CTRL_4BITBUS;
         }
 
         ret  = hostctl1;
@@ -1630,21 +1630,21 @@ static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
 
     case SDHC_PRNSTS:
         /* Add SDSTB (SD Clock Stable) bit to PRNSTS */
-        ret = sdhci_read(opaque, offset, size) & ~ESDHC_PRNSTS_SDSTB;
+        ret = sdhci_read(opaque, offset, size) & ~USDHC_PRNSTS_SDSTB;
         if (s->clkcon & SDHC_CLOCK_INT_STABLE) {
-            ret |= ESDHC_PRNSTS_SDSTB;
+            ret |= USDHC_PRNSTS_SDSTB;
         }
         break;
 
-    case ESDHC_VENDOR_SPEC:
+    case USDHC_VENDOR_SPEC:
         ret = s->vendor_spec;
         break;
-    case ESDHC_DLL_CTRL:
-    case ESDHC_TUNE_CTRL_STATUS:
-    case ESDHC_UNDOCUMENTED_REG27:
-    case ESDHC_TUNING_CTRL:
-    case ESDHC_MIX_CTRL:
-    case ESDHC_WTMK_LVL:
+    case USDHC_DLL_CTRL:
+    case USDHC_TUNE_CTRL_STATUS:
+    case USDHC_UNDOCUMENTED_REG27:
+    case USDHC_TUNING_CTRL:
+    case USDHC_MIX_CTRL:
+    case USDHC_WTMK_LVL:
         ret = 0;
         break;
     }
@@ -1660,18 +1660,18 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
     uint32_t value = (uint32_t)val;
 
     switch (offset) {
-    case ESDHC_DLL_CTRL:
-    case ESDHC_TUNE_CTRL_STATUS:
-    case ESDHC_UNDOCUMENTED_REG27:
-    case ESDHC_TUNING_CTRL:
-    case ESDHC_WTMK_LVL:
+    case USDHC_DLL_CTRL:
+    case USDHC_TUNE_CTRL_STATUS:
+    case USDHC_UNDOCUMENTED_REG27:
+    case USDHC_TUNING_CTRL:
+    case USDHC_WTMK_LVL:
         break;
 
-    case ESDHC_VENDOR_SPEC:
+    case USDHC_VENDOR_SPEC:
         s->vendor_spec = value;
         switch (s->vendor) {
         case SDHCI_VENDOR_IMX:
-            if (value & ESDHC_IMX_FRC_SDCLK_ON) {
+            if (value & USDHC_IMX_FRC_SDCLK_ON) {
                 s->prnsts &= ~SDHC_IMX_CLOCK_GATE_OFF;
             } else {
                 s->prnsts |= SDHC_IMX_CLOCK_GATE_OFF;
@@ -1740,12 +1740,12 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
          * Second, split "Data Transfer Width" from bits 2 and 1 in to
          * bits 5 and 1
          */
-        if (value & ESDHC_CTRL_8BITBUS) {
+        if (value & USDHC_CTRL_8BITBUS) {
             hostctl1 |= SDHC_CTRL_8BITBUS;
         }
 
-        if (value & ESDHC_CTRL_4BITBUS) {
-            hostctl1 |= ESDHC_CTRL_4BITBUS;
+        if (value & USDHC_CTRL_4BITBUS) {
+            hostctl1 |= USDHC_CTRL_4BITBUS;
         }
 
         /*
@@ -1768,7 +1768,7 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         sdhci_write(opaque, offset, value, size);
         break;
 
-    case ESDHC_MIX_CTRL:
+    case USDHC_MIX_CTRL:
         /*
          * So, when SD/MMC stack in Linux tries to write to "Transfer
          * Mode Register", ESDHC i.MX quirk code will translate it
-- 
2.38.0

[PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[Bernhard Beschow]

Allows e500 boards to have their root file system reside on flash using
only builtin devices located in the eLBC memory region.

Note that the flash memory area is only created when a -pflash argument is
given, and that the size is determined by the given file. The idea is to
put users into control.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 docs/system/ppc/ppce500.rst | 16 ++++++++
 hw/ppc/Kconfig              |  1 +
 hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+)

diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
index 7b5eb3c4ee..38f8ceb0cf 100644
--- a/docs/system/ppc/ppce500.rst
+++ b/docs/system/ppc/ppce500.rst
@@ -165,3 +165,19 @@ if “-device eTSEC” is given to QEMU:
 .. code-block:: bash
 
   -netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device eTSEC,netdev=net0
+
+Root file system on flash drive
+-------------------------------
+
+Rather than using a root file system on ram disk, it is possible to have it on
+CFI flash. Given an ext2 image whose size must be a power of two, it can be used
+as follows:
+
+.. code-block:: bash
+
+  $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
+      -display none -serial stdio \
+      -kernel vmlinux \
+      -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
+      -append "rootwait root=/dev/mtdblock0"
+
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 791fe78a50..769a1ead1c 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -126,6 +126,7 @@ config E500
     select ETSEC
     select GPIO_MPC8XXX
     select OPENPIC
+    select PFLASH_CFI01
     select PLATFORM_BUS
     select PPCE500_PCI
     select SERIAL
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 3e950ea3ba..73198adac8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -23,8 +23,10 @@
 #include "e500-ccsr.h"
 #include "net/net.h"
 #include "qemu/config-file.h"
+#include "hw/block/flash.h"
 #include "hw/char/serial.h"
 #include "hw/pci/pci.h"
+#include "sysemu/block-backend-io.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/reset.h"
@@ -267,6 +269,31 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
     }
 }
 
+static void create_devtree_flash(SysBusDevice *sbdev,
+                                 PlatformDevtreeData *data)
+{
+    g_autofree char *name = NULL;
+    uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
+                                                   "num-blocks",
+                                                   &error_fatal);
+    uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
+                                                      "sector-length",
+                                                      &error_fatal);
+    uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
+                                                   "width",
+                                                   &error_fatal);
+    hwaddr flashbase = 0;
+    hwaddr flashsize = num_blocks * sector_length;
+    void *fdt = data->fdt;
+
+    name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
+    qemu_fdt_add_subnode(fdt, name);
+    qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
+    qemu_fdt_setprop_sized_cells(fdt, name, "reg",
+                                 1, flashbase, 1, flashsize);
+    qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
+}
+
 static void platform_bus_create_devtree(PPCE500MachineState *pms,
                                         void *fdt, const char *mpic)
 {
@@ -276,6 +303,8 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
     uint64_t addr = pmc->platform_bus_base;
     uint64_t size = pmc->platform_bus_size;
     int irq_start = pmc->platform_bus_first_irq;
+    SysBusDevice *sbdev;
+    bool ambiguous;
 
     /* Create a /platform node that we can put all devices into */
 
@@ -302,6 +331,13 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
     /* Loop through all dynamic sysbus devices and create nodes for them */
     foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
 
+    sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
+                                                    &ambiguous));
+    if (sbdev) {
+        assert(!ambiguous);
+        create_devtree_flash(sbdev, &data);
+    }
+
     g_free(node);
 }
 
@@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
     unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
     IrqLines *irqs;
     DeviceState *dev, *mpicdev;
+    DriveInfo *dinfo;
     CPUPPCState *firstenv = NULL;
     MemoryRegion *ccsr_addr_space;
     SysBusDevice *s;
@@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
                                 pmc->platform_bus_base,
                                 &pms->pbus_dev->mmio);
 
+    dinfo = drive_get(IF_PFLASH, 0, 0);
+    if (dinfo) {
+        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
+        BlockDriverState *bs = blk_bs(blk);
+        uint64_t size = bdrv_getlength(bs);
+        uint64_t mmio_size = pms->pbus_dev->mmio.size;
+        uint32_t sector_len = 64 * KiB;
+
+        if (!is_power_of_2(size)) {
+            error_report("Size of pflash file must be a power of two.");
+            exit(1);
+        }
+
+        if (size > mmio_size) {
+            error_report("Size of pflash file must not be bigger than %" PRIu64
+                         " bytes.", mmio_size);
+            exit(1);
+        }
+
+        if (!QEMU_IS_ALIGNED(size, sector_len)) {
+            error_report("Size of pflash file must be a multiple of %" PRIu32
+                         ".", sector_len);
+            exit(1);
+        }
+
+        dev = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_drive(dev, "drive", blk);
+        qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
+        qdev_prop_set_uint64(dev, "sector-length", sector_len);
+        qdev_prop_set_uint8(dev, "width", 2);
+        qdev_prop_set_bit(dev, "big-endian", true);
+        qdev_prop_set_uint16(dev, "id0", 0x89);
+        qdev_prop_set_uint16(dev, "id1", 0x18);
+        qdev_prop_set_uint16(dev, "id2", 0x0000);
+        qdev_prop_set_uint16(dev, "id3", 0x0);
+        qdev_prop_set_string(dev, "name", "e500.flash");
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
+        memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
+                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
+    }
+
     /*
      * Smart firmware defaults ahead!
      *
-- 
2.38.0

[PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Bernhard Beschow]

Will allow e500 boards to access SD cards using just their own devices.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/sd/sdhci.c         | 120 +++++++++++++++++++++++++++++++++++++++++-
 include/hw/sd/sdhci.h |   3 ++
 2 files changed, 122 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 306070c872..8d8ad9ff24 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
 
     s->io_ops = &sdhci_mmio_ops;
+    s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
 }
 
 void sdhci_uninitfn(SDHCIState *s)
@@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
 
     memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
-                          SDHC_REGISTERS_MAP_SIZE);
+                          s->io_registers_map_size);
 }
 
 void sdhci_common_unrealize(SDHCIState *s)
@@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = {
     .class_init = sdhci_bus_class_init,
 };
 
+/* --- qdev Freescale eSDHC --- */
+
+/* Watermark Level Register */
+#define ESDHC_WML                    0x44
+
+/* Control Register for DMA transfer */
+#define ESDHC_DMA_SYSCTL            0x40c
+
+#define ESDHC_REGISTERS_MAP_SIZE    0x410
+
+static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
+{
+    uint64_t ret;
+
+    switch (offset) {
+    case SDHC_SYSAD:
+    case SDHC_BLKSIZE:
+    case SDHC_ARGUMENT:
+    case SDHC_TRNMOD:
+    case SDHC_RSPREG0:
+    case SDHC_RSPREG1:
+    case SDHC_RSPREG2:
+    case SDHC_RSPREG3:
+    case SDHC_BDATA:
+    case SDHC_PRNSTS:
+    case SDHC_HOSTCTL:
+    case SDHC_CLKCON:
+    case SDHC_NORINTSTS:
+    case SDHC_NORINTSTSEN:
+    case SDHC_NORINTSIGEN:
+    case SDHC_ACMD12ERRSTS:
+    case SDHC_CAPAB:
+    case SDHC_SLOT_INT_STATUS:
+        ret = sdhci_read(opaque, offset, size);
+        break;
+
+    case ESDHC_WML:
+    case ESDHC_DMA_SYSCTL:
+        ret = 0;
+        qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx
+                      " not implemented\n", offset);
+        break;
+
+    default:
+        ret = 0;
+        qemu_log_mask(LOG_GUEST_ERROR, "ESDHC rd @0x%02" HWADDR_PRIx
+                      " unknown offset\n", offset);
+        break;
+    }
+
+    return ret;
+}
+
+static void esdhci_write(void *opaque, hwaddr offset, uint64_t val,
+                         unsigned size)
+{
+    switch (offset) {
+    case SDHC_SYSAD:
+    case SDHC_BLKSIZE:
+    case SDHC_ARGUMENT:
+    case SDHC_TRNMOD:
+    case SDHC_BDATA:
+    case SDHC_HOSTCTL:
+    case SDHC_CLKCON:
+    case SDHC_NORINTSTS:
+    case SDHC_NORINTSTSEN:
+    case SDHC_NORINTSIGEN:
+    case SDHC_FEAER:
+        sdhci_write(opaque, offset, val, size);
+        break;
+
+    case ESDHC_WML:
+    case ESDHC_DMA_SYSCTL:
+        qemu_log_mask(LOG_UNIMP, "ESDHC wr @0x%02" HWADDR_PRIx " <- 0x%08lx "
+                      "not implemented\n", offset, val);
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "ESDHC wr @0x%02" HWADDR_PRIx
+                      " <- 0x%08lx unknown offset\n", offset, val);
+        break;
+    }
+}
+
+static const MemoryRegionOps esdhc_mmio_ops = {
+    .read = esdhci_read,
+    .write = esdhci_write,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+        .unaligned = false
+    },
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void esdhci_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+    SDHCIState *s = SYSBUS_SDHCI(obj);
+
+    s->io_ops = &esdhc_mmio_ops;
+    s->io_registers_map_size = ESDHC_REGISTERS_MAP_SIZE;
+
+    /*
+     * Compatible with:
+     * - SD Host Controller Specification Version 2.0 Part A2
+     */
+    qdev_prop_set_uint8(dev, "sd-spec-version", 2);
+}
+
+static const TypeInfo esdhc_info = {
+    .name = TYPE_FSL_ESDHC,
+    .parent = TYPE_SYSBUS_SDHCI,
+    .instance_init = esdhci_init,
+};
+
 /* --- qdev i.MX eSDHC --- */
 
 #define USDHC_MIX_CTRL                  0x48
@@ -1907,6 +2024,7 @@ static void sdhci_register_types(void)
 {
     type_register_static(&sdhci_sysbus_info);
     type_register_static(&sdhci_bus_info);
+    type_register_static(&esdhc_info);
     type_register_static(&imx_usdhc_info);
     type_register_static(&sdhci_s3c_info);
 }
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 01a64c5442..5b32e83eee 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -45,6 +45,7 @@ struct SDHCIState {
     AddressSpace *dma_as;
     MemoryRegion *dma_mr;
     const MemoryRegionOps *io_ops;
+    uint64_t io_registers_map_size;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
@@ -122,6 +123,8 @@ DECLARE_INSTANCE_CHECKER(SDHCIState, PCI_SDHCI,
 DECLARE_INSTANCE_CHECKER(SDHCIState, SYSBUS_SDHCI,
                          TYPE_SYSBUS_SDHCI)
 
+#define TYPE_FSL_ESDHC "fsl-esdhc"
+
 #define TYPE_IMX_USDHC "imx-usdhc"
 
 #define TYPE_S3C_SDHCI "s3c-sdhci"
-- 
2.38.0

[PATCH v4 7/7] hw/ppc/e500: Add Freescale eSDHC to e500plat

[Bernhard Beschow]

Adds missing functionality to e500plat machine which increases the
chance of given "real" firmware images to access SD cards.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 docs/system/ppc/ppce500.rst | 12 ++++++++++++
 hw/ppc/Kconfig              |  1 +
 hw/ppc/e500.c               | 35 ++++++++++++++++++++++++++++++++++-
 hw/ppc/e500.h               |  1 +
 hw/ppc/e500plat.c           |  1 +
 5 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
index 38f8ceb0cf..c9fe0915dc 100644
--- a/docs/system/ppc/ppce500.rst
+++ b/docs/system/ppc/ppce500.rst
@@ -19,6 +19,7 @@ The ``ppce500`` machine supports the following devices:
 * Power-off functionality via one GPIO pin
 * 1 Freescale MPC8xxx PCI host controller
 * VirtIO devices via PCI bus
+* 1 Freescale Enhanced Secure Digital Host controller (eSDHC)
 * 1 Freescale Enhanced Triple Speed Ethernet controller (eTSEC)
 
 Hardware configuration information
@@ -181,3 +182,14 @@ as follows:
       -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
       -append "rootwait root=/dev/mtdblock0"
 
+Alternatively, the root file system can also reside on an emulated SD card
+whose size must again be a power of two:
+
+.. code-block:: bash
+
+  $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
+      -display none -serial stdio \
+      -kernel vmlinux \
+      -device sd-card,drive=mydrive \
+      -drive id=mydrive,if=none,file=/path/to/rootfs.ext2,format=raw \
+      -append "rootwait root=/dev/mmcblk0"
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 769a1ead1c..6e31f568ba 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -129,6 +129,7 @@ config E500
     select PFLASH_CFI01
     select PLATFORM_BUS
     select PPCE500_PCI
+    select SDHCI
     select SERIAL
     select MPC_I2C
     select FDT_PPC
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 73198adac8..15d1f5ea00 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -48,6 +48,7 @@
 #include "hw/net/fsl_etsec/etsec.h"
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
+#include "hw/sd/sdhci.h"
 
 #define EPAPR_MAGIC                (0x45504150)
 #define DTC_LOAD_PAD               0x1800000
@@ -66,11 +67,14 @@
 #define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
 #define MPC8544_PCI_REGS_OFFSET    0x8000ULL
 #define MPC8544_PCI_REGS_SIZE      0x1000ULL
+#define MPC85XX_ESDHC_REGS_OFFSET  0x2e000ULL
+#define MPC85XX_ESDHC_REGS_SIZE    0x1000ULL
 #define MPC8544_UTIL_OFFSET        0xe0000ULL
 #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
 #define MPC8544_I2C_REGS_OFFSET    0x3000ULL
 #define MPC8XXX_GPIO_IRQ           47
 #define MPC8544_I2C_IRQ            43
+#define MPC85XX_ESDHC_IRQ          72
 #define RTC_REGS_OFFSET            0x68
 
 #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
@@ -203,6 +207,22 @@ static void dt_i2c_create(void *fdt, const char *soc, const char *mpic,
     g_free(i2c);
 }
 
+static void dt_sdhc_create(void *fdt, const char *parent, const char *mpic)
+{
+    hwaddr mmio = MPC85XX_ESDHC_REGS_OFFSET;
+    hwaddr size = MPC85XX_ESDHC_REGS_SIZE;
+    int irq = MPC85XX_ESDHC_IRQ;
+    g_autofree char *name = NULL;
+
+    name = g_strdup_printf("%s/sdhc@%" PRIx64, parent, mmio);
+    qemu_fdt_add_subnode(fdt, name);
+    qemu_fdt_setprop(fdt, name, "sdhci,auto-cmd12", NULL, 0);
+    qemu_fdt_setprop_phandle(fdt, name, "interrupt-parent", mpic);
+    qemu_fdt_setprop_cells(fdt, name, "bus-width", 4);
+    qemu_fdt_setprop_cells(fdt, name, "interrupts", irq, 0x2);
+    qemu_fdt_setprop_cells(fdt, name, "reg", mmio, size);
+    qemu_fdt_setprop_string(fdt, name, "compatible", "fsl,esdhc");
+}
 
 typedef struct PlatformDevtreeData {
     void *fdt;
@@ -553,6 +573,10 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
 
     dt_rtc_create(fdt, "i2c", "rtc");
 
+    /* sdhc */
+    if (pmc->has_esdhc) {
+        dt_sdhc_create(fdt, soc, mpic);
+    }
 
     gutil = g_strdup_printf("%s/global-utilities@%llx", soc,
                             MPC8544_UTIL_OFFSET);
@@ -982,7 +1006,8 @@ void ppce500_init(MachineState *machine)
                        0, qdev_get_gpio_in(mpicdev, 42), 399193,
                        serial_hd(1), DEVICE_BIG_ENDIAN);
     }
-        /* I2C */
+
+    /* I2C */
     dev = qdev_new("mpc-i2c");
     s = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(s, &error_fatal);
@@ -992,6 +1017,14 @@ void ppce500_init(MachineState *machine)
     i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
     i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
 
+    /* eSDHC */
+    if (pmc->has_esdhc) {
+        dev = qdev_new(TYPE_FSL_ESDHC);
+        s = SYS_BUS_DEVICE(dev);
+        sysbus_realize_and_unref(s, &error_fatal);
+        sysbus_mmio_map(s, 0, pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET);
+        sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
+    }
 
     /* General Utility device */
     dev = qdev_new("mpc8544-guts");
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 68f754ce50..8c09ef92e4 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -27,6 +27,7 @@ struct PPCE500MachineClass {
 
     int mpic_version;
     bool has_mpc8xxx_gpio;
+    bool has_esdhc;
     hwaddr platform_bus_base;
     hwaddr platform_bus_size;
     int platform_bus_first_irq;
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 5bb1c603da..44bf874b0f 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -86,6 +86,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
     pmc->fixup_devtree = e500plat_fixup_devtree;
     pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
     pmc->has_mpc8xxx_gpio = true;
+    pmc->has_esdhc = true;
     pmc->platform_bus_base = 0xf00000000ULL;
     pmc->platform_bus_size = 128 * MiB;
     pmc->platform_bus_first_irq = 5;
-- 
2.38.0

Re: [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup

[Bernhard Beschow]

Am 18. Oktober 2022 21:01:39 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Cover letter:
>
>~~~~~~~~~~~~~
>
>
>
>This series adds support for -pflash and direct SD card access to the
>
>PPC e500 boards. The idea is to increase compatibility with "real" firmware
>
>images where only the bare minimum of drivers is compiled in.
>
>
>
>The series is structured as follows:
>
>
>
>Patches 1-4 perform some general cleanup which paves the way for the rest of
>
>the series.
>
>
>
>Patch 5 adds -pflash handling where memory-mapped flash can be added on
>
>user's behalf. That is, the flash memory region in the eLBC is only added if
>
>the -pflash argument is supplied. Note that the cfi01 device model becomes
>
>stricter in checking the size of the emulated flash space.
>
>
>
>Patches 6 and 7 add a new device model - the Freescale eSDHC - to the e500
>
>boards which was missing so far.
>
>
>
>User documentation is also added as the new features become available.
>
>
>
>Tesing done:
>
>* `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
>
>"console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive
>
>if=pflash,file=rootfs.ext2,format=raw`
>
>* `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
>
>"console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive
>
>id=mydrive,if=none,file=rootfs.ext2,format=raw`
>
>
>
>The load was created using latest Buildroot with `make
>
>qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type.
>
>In both cases it was possible to log in and explore the root file system.
>
>
>
>v4:
>
>~~~
>
>Zoltan:
>
>- Don't suggest presence of qemu-system-ppc32 in documentation
>
>
>
>Bin:
>
>- New patch: docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
>
>
>
>Peter:
>
>- Inline pflash_cfi01_register() rather than modify it (similar to v2)
>

Ping

>
>
>v3:
>
>~~~
>
>Phil:
>
>- Also add power-of-2 fix to pflash_cfi02
>
>- Resolve cfi01-specific assertion in e500 code
>
>- Resolve unused define in eSDHC device model
>
>- Resolve redundant alignment checks in eSDHC device model
>
>
>
>Bin:
>
>- Add dedicated flash chapter to documentation
>
>
>
>Bernhard:
>
>- Use is_power_of_2() instead of ctpop64() for better readability
>
>- Only instantiate eSDHC device model in ppce500 (not used in MPC8544DS)
>
>- Rebase onto gitlab.com/danielhb/qemu/tree/ppc-next
>
>
>
>v2:
>
>~~~
>
>Bin:
>
>- Add source for MPC8544DS platform bus' memory map in commit message.
>
>- Keep "ESDHC" in comment referring to Linux driver.
>
>- Use "qemu-system-ppc{64|32} in documentation.
>
>- Use g_autofree in device tree code.
>
>- Remove unneeded device tree properties.
>
>- Error out if pflash size doesn't fit into eLBC memory window.
>
>- Remove unused ESDHC defines.
>
>- Define macro ESDHC_WML for register offset with magic constant.
>
>- Fix some whitespace issues when adding eSDHC device to e500.
>
>
>
>Phil:
>
>- Fix tense in commit message.
>
>
>
>Bernhard Beschow (7):
>
>  docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
>
>  hw/block/pflash_cfi0{1,2}: Error out if device length isn't a power of
>
>    two
>
>  hw/sd/sdhci-internal: Unexport ESDHC defines
>
>  hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
>
>  hw/ppc/e500: Implement pflash handling
>
>  hw/sd/sdhci: Implement Freescale eSDHC device model
>
>  hw/ppc/e500: Add Freescale eSDHC to e500plat
>
>
>
> docs/system/ppc/ppce500.rst |  38 +++++++-
>
> hw/block/pflash_cfi01.c     |   8 +-
>
> hw/block/pflash_cfi02.c     |   5 +
>
> hw/ppc/Kconfig              |   2 +
>
> hw/ppc/e500.c               | 114 +++++++++++++++++++++-
>
> hw/ppc/e500.h               |   1 +
>
> hw/ppc/e500plat.c           |   1 +
>
> hw/sd/sdhci-internal.h      |  20 ----
>
> hw/sd/sdhci.c               | 183 +++++++++++++++++++++++++++++++-----
>
> include/hw/sd/sdhci.h       |   3 +
>
> 10 files changed, 324 insertions(+), 51 deletions(-)
>
>
>
>-- >
>2.38.0
>
>
>

Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[Daniel Henrique Barboza]

On 10/18/22 18:01, Bernhard Beschow wrote:
> Allows e500 boards to have their root file system reside on flash using
> only builtin devices located in the eLBC memory region.
> 
> Note that the flash memory area is only created when a -pflash argument is
> given, and that the size is determined by the given file. The idea is to
> put users into control.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   docs/system/ppc/ppce500.rst | 16 ++++++++
>   hw/ppc/Kconfig              |  1 +
>   hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 96 insertions(+)
> 
> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> index 7b5eb3c4ee..38f8ceb0cf 100644
> --- a/docs/system/ppc/ppce500.rst
> +++ b/docs/system/ppc/ppce500.rst
> @@ -165,3 +165,19 @@ if “-device eTSEC” is given to QEMU:
>   .. code-block:: bash
>   
>     -netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device eTSEC,netdev=net0
> +
> +Root file system on flash drive
> +-------------------------------
> +
> +Rather than using a root file system on ram disk, it is possible to have it on
> +CFI flash. Given an ext2 image whose size must be a power of two, it can be used
> +as follows:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
> +      -display none -serial stdio \
> +      -kernel vmlinux \
> +      -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
> +      -append "rootwait root=/dev/mtdblock0"
> +
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 791fe78a50..769a1ead1c 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -126,6 +126,7 @@ config E500
>       select ETSEC
>       select GPIO_MPC8XXX
>       select OPENPIC
> +    select PFLASH_CFI01
>       select PLATFORM_BUS
>       select PPCE500_PCI
>       select SERIAL
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 3e950ea3ba..73198adac8 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -23,8 +23,10 @@
>   #include "e500-ccsr.h"
>   #include "net/net.h"
>   #include "qemu/config-file.h"
> +#include "hw/block/flash.h"
>   #include "hw/char/serial.h"
>   #include "hw/pci/pci.h"
> +#include "sysemu/block-backend-io.h"
>   #include "sysemu/sysemu.h"
>   #include "sysemu/kvm.h"
>   #include "sysemu/reset.h"
> @@ -267,6 +269,31 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
>       }
>   }
>   
> +static void create_devtree_flash(SysBusDevice *sbdev,
> +                                 PlatformDevtreeData *data)
> +{
> +    g_autofree char *name = NULL;
> +    uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
> +                                                   "num-blocks",
> +                                                   &error_fatal);
> +    uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
> +                                                      "sector-length",
> +                                                      &error_fatal);
> +    uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
> +                                                   "width",
> +                                                   &error_fatal);
> +    hwaddr flashbase = 0;
> +    hwaddr flashsize = num_blocks * sector_length;
> +    void *fdt = data->fdt;
> +
> +    name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
> +    qemu_fdt_add_subnode(fdt, name);
> +    qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
> +    qemu_fdt_setprop_sized_cells(fdt, name, "reg",
> +                                 1, flashbase, 1, flashsize);
> +    qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
> +}
> +
>   static void platform_bus_create_devtree(PPCE500MachineState *pms,
>                                           void *fdt, const char *mpic)
>   {
> @@ -276,6 +303,8 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
>       uint64_t addr = pmc->platform_bus_base;
>       uint64_t size = pmc->platform_bus_size;
>       int irq_start = pmc->platform_bus_first_irq;
> +    SysBusDevice *sbdev;
> +    bool ambiguous;
>   
>       /* Create a /platform node that we can put all devices into */
>   
> @@ -302,6 +331,13 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
>       /* Loop through all dynamic sysbus devices and create nodes for them */
>       foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
>   
> +    sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
> +                                                    &ambiguous));
> +    if (sbdev) {
> +        assert(!ambiguous);
> +        create_devtree_flash(sbdev, &data);
> +    }
> +
>       g_free(node);
>   }
>   
> @@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
>       unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
>       IrqLines *irqs;
>       DeviceState *dev, *mpicdev;
> +    DriveInfo *dinfo;
>       CPUPPCState *firstenv = NULL;
>       MemoryRegion *ccsr_addr_space;
>       SysBusDevice *s;
> @@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
>                                   pmc->platform_bus_base,
>                                   &pms->pbus_dev->mmio);
>   
> +    dinfo = drive_get(IF_PFLASH, 0, 0);
> +    if (dinfo) {
> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> +        BlockDriverState *bs = blk_bs(blk);
> +        uint64_t size = bdrv_getlength(bs);
> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;
> +        uint32_t sector_len = 64 * KiB;
> +
> +        if (!is_power_of_2(size)) {
> +            error_report("Size of pflash file must be a power of two.");
> +            exit(1);
> +        }
> +
> +        if (size > mmio_size) {
> +            error_report("Size of pflash file must not be bigger than %" PRIu64
> +                         " bytes.", mmio_size);
> +            exit(1);
> +        }
> +
> +        if (!QEMU_IS_ALIGNED(size, sector_len)) {
> +            error_report("Size of pflash file must be a multiple of %" PRIu32
> +                         ".", sector_len);
> +            exit(1);
> +        }
> +
> +        dev = qdev_new(TYPE_PFLASH_CFI01);
> +        qdev_prop_set_drive(dev, "drive", blk);
> +        qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
> +        qdev_prop_set_uint64(dev, "sector-length", sector_len);
> +        qdev_prop_set_uint8(dev, "width", 2);
> +        qdev_prop_set_bit(dev, "big-endian", true);
> +        qdev_prop_set_uint16(dev, "id0", 0x89);
> +        qdev_prop_set_uint16(dev, "id1", 0x18);
> +        qdev_prop_set_uint16(dev, "id2", 0x0000);
> +        qdev_prop_set_uint16(dev, "id3", 0x0);
> +        qdev_prop_set_string(dev, "name", "e500.flash");
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +
> +        memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
> +                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
> +    }
> +
>       /*
>        * Smart firmware defaults ahead!
>        *

Re: [PATCH v4 7/7] hw/ppc/e500: Add Freescale eSDHC to e500plat

[Daniel Henrique Barboza]

On 10/18/22 18:01, Bernhard Beschow wrote:
> Adds missing functionality to e500plat machine which increases the
> chance of given "real" firmware images to access SD cards.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   docs/system/ppc/ppce500.rst | 12 ++++++++++++
>   hw/ppc/Kconfig              |  1 +
>   hw/ppc/e500.c               | 35 ++++++++++++++++++++++++++++++++++-
>   hw/ppc/e500.h               |  1 +
>   hw/ppc/e500plat.c           |  1 +
>   5 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> index 38f8ceb0cf..c9fe0915dc 100644
> --- a/docs/system/ppc/ppce500.rst
> +++ b/docs/system/ppc/ppce500.rst
> @@ -19,6 +19,7 @@ The ``ppce500`` machine supports the following devices:
>   * Power-off functionality via one GPIO pin
>   * 1 Freescale MPC8xxx PCI host controller
>   * VirtIO devices via PCI bus
> +* 1 Freescale Enhanced Secure Digital Host controller (eSDHC)
>   * 1 Freescale Enhanced Triple Speed Ethernet controller (eTSEC)
>   
>   Hardware configuration information
> @@ -181,3 +182,14 @@ as follows:
>         -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
>         -append "rootwait root=/dev/mtdblock0"
>   
> +Alternatively, the root file system can also reside on an emulated SD card
> +whose size must again be a power of two:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
> +      -display none -serial stdio \
> +      -kernel vmlinux \
> +      -device sd-card,drive=mydrive \
> +      -drive id=mydrive,if=none,file=/path/to/rootfs.ext2,format=raw \
> +      -append "rootwait root=/dev/mmcblk0"
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 769a1ead1c..6e31f568ba 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -129,6 +129,7 @@ config E500
>       select PFLASH_CFI01
>       select PLATFORM_BUS
>       select PPCE500_PCI
> +    select SDHCI
>       select SERIAL
>       select MPC_I2C
>       select FDT_PPC
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 73198adac8..15d1f5ea00 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -48,6 +48,7 @@
>   #include "hw/net/fsl_etsec/etsec.h"
>   #include "hw/i2c/i2c.h"
>   #include "hw/irq.h"
> +#include "hw/sd/sdhci.h"
>   
>   #define EPAPR_MAGIC                (0x45504150)
>   #define DTC_LOAD_PAD               0x1800000
> @@ -66,11 +67,14 @@
>   #define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
>   #define MPC8544_PCI_REGS_OFFSET    0x8000ULL
>   #define MPC8544_PCI_REGS_SIZE      0x1000ULL
> +#define MPC85XX_ESDHC_REGS_OFFSET  0x2e000ULL
> +#define MPC85XX_ESDHC_REGS_SIZE    0x1000ULL
>   #define MPC8544_UTIL_OFFSET        0xe0000ULL
>   #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
>   #define MPC8544_I2C_REGS_OFFSET    0x3000ULL
>   #define MPC8XXX_GPIO_IRQ           47
>   #define MPC8544_I2C_IRQ            43
> +#define MPC85XX_ESDHC_IRQ          72
>   #define RTC_REGS_OFFSET            0x68
>   
>   #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
> @@ -203,6 +207,22 @@ static void dt_i2c_create(void *fdt, const char *soc, const char *mpic,
>       g_free(i2c);
>   }
>   
> +static void dt_sdhc_create(void *fdt, const char *parent, const char *mpic)
> +{
> +    hwaddr mmio = MPC85XX_ESDHC_REGS_OFFSET;
> +    hwaddr size = MPC85XX_ESDHC_REGS_SIZE;
> +    int irq = MPC85XX_ESDHC_IRQ;
> +    g_autofree char *name = NULL;
> +
> +    name = g_strdup_printf("%s/sdhc@%" PRIx64, parent, mmio);
> +    qemu_fdt_add_subnode(fdt, name);
> +    qemu_fdt_setprop(fdt, name, "sdhci,auto-cmd12", NULL, 0);
> +    qemu_fdt_setprop_phandle(fdt, name, "interrupt-parent", mpic);
> +    qemu_fdt_setprop_cells(fdt, name, "bus-width", 4);
> +    qemu_fdt_setprop_cells(fdt, name, "interrupts", irq, 0x2);
> +    qemu_fdt_setprop_cells(fdt, name, "reg", mmio, size);
> +    qemu_fdt_setprop_string(fdt, name, "compatible", "fsl,esdhc");
> +}
>   
>   typedef struct PlatformDevtreeData {
>       void *fdt;
> @@ -553,6 +573,10 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
>   
>       dt_rtc_create(fdt, "i2c", "rtc");
>   
> +    /* sdhc */
> +    if (pmc->has_esdhc) {
> +        dt_sdhc_create(fdt, soc, mpic);
> +    }
>   
>       gutil = g_strdup_printf("%s/global-utilities@%llx", soc,
>                               MPC8544_UTIL_OFFSET);
> @@ -982,7 +1006,8 @@ void ppce500_init(MachineState *machine)
>                          0, qdev_get_gpio_in(mpicdev, 42), 399193,
>                          serial_hd(1), DEVICE_BIG_ENDIAN);
>       }
> -        /* I2C */
> +
> +    /* I2C */
>       dev = qdev_new("mpc-i2c");
>       s = SYS_BUS_DEVICE(dev);
>       sysbus_realize_and_unref(s, &error_fatal);
> @@ -992,6 +1017,14 @@ void ppce500_init(MachineState *machine)
>       i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
>       i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
>   
> +    /* eSDHC */
> +    if (pmc->has_esdhc) {
> +        dev = qdev_new(TYPE_FSL_ESDHC);
> +        s = SYS_BUS_DEVICE(dev);
> +        sysbus_realize_and_unref(s, &error_fatal);
> +        sysbus_mmio_map(s, 0, pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET);
> +        sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
> +    }
>   
>       /* General Utility device */
>       dev = qdev_new("mpc8544-guts");
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 68f754ce50..8c09ef92e4 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -27,6 +27,7 @@ struct PPCE500MachineClass {
>   
>       int mpic_version;
>       bool has_mpc8xxx_gpio;
> +    bool has_esdhc;
>       hwaddr platform_bus_base;
>       hwaddr platform_bus_size;
>       int platform_bus_first_irq;
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 5bb1c603da..44bf874b0f 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -86,6 +86,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>       pmc->fixup_devtree = e500plat_fixup_devtree;
>       pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
>       pmc->has_mpc8xxx_gpio = true;
> +    pmc->has_esdhc = true;
>       pmc->platform_bus_base = 0xf00000000ULL;
>       pmc->platform_bus_size = 128 * MiB;
>       pmc->platform_bus_first_irq = 5;

Re: [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup

[Daniel Henrique Barboza]

Hi,

Since this is being sent to qemu-ppc and has to do with e500 I decided to
take a look. I acked the e500 related patches, 5 and 7. Patch 6 LGTM as well
but I'd rather not ack it it's SD specific code.

I'll send a PowerPC pull request this week. I can grab this series via the ppc
tree if someone with SD authority acks patch 6.


Thanks,


Daniel





On 10/18/22 18:01, Bernhard Beschow wrote:
> Cover letter:
> ~~~~~~~~~~~~~
> 
> This series adds support for -pflash and direct SD card access to the
> PPC e500 boards. The idea is to increase compatibility with "real" firmware
> images where only the bare minimum of drivers is compiled in.
> 
> The series is structured as follows:
> 
> Patches 1-4 perform some general cleanup which paves the way for the rest of
> the series.
> 
> Patch 5 adds -pflash handling where memory-mapped flash can be added on
> user's behalf. That is, the flash memory region in the eLBC is only added if
> the -pflash argument is supplied. Note that the cfi01 device model becomes
> stricter in checking the size of the emulated flash space.
> 
> Patches 6 and 7 add a new device model - the Freescale eSDHC - to the e500
> boards which was missing so far.
> 
> User documentation is also added as the new features become available.
> 
> Tesing done:
> * `qeu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
> "console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive
> if=pflash,file=rootfs.ext2,format=raw`
> * `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
> "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive
> id=mydrive,if=none,file=rootfs.ext2,format=raw`
> 
> The load was created using latest Buildroot with `make
> qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type.
> In both cases it was possible to log in and explore the root file system.
> 
> v4:
> ~~~
> Zoltan:
> - Don't suggest presence of qemu-system-ppc32 in documentation
> 
> Bin:
> - New patch: docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
> 
> Peter:
> - Inline pflash_cfi01_register() rather than modify it (similar to v2)
> 
> v3:
> ~~~
> Phil:
> - Also add power-of-2 fix to pflash_cfi02
> - Resolve cfi01-specific assertion in e500 code
> - Resolve unused define in eSDHC device model
> - Resolve redundant alignment checks in eSDHC device model
> 
> Bin:
> - Add dedicated flash chapter to documentation
> 
> Bernhard:
> - Use is_power_of_2() instead of ctpop64() for better readability
> - Only instantiate eSDHC device model in ppce500 (not used in MPC8544DS)
> - Rebase onto gitlab.com/danielhb/qemu/tree/ppc-next
> 
> v2:
> ~~~
> Bin:
> - Add source for MPC8544DS platform bus' memory map in commit message.
> - Keep "ESDHC" in comment referring to Linux driver.
> - Use "qemu-system-ppc{64|32} in documentation.
> - Use g_autofree in device tree code.
> - Remove unneeded device tree properties.
> - Error out if pflash size doesn't fit into eLBC memory window.
> - Remove unused ESDHC defines.
> - Define macro ESDHC_WML for register offset with magic constant.
> - Fix some whitespace issues when adding eSDHC device to e500.
> 
> Phil:
> - Fix tense in commit message.
> 
> Bernhard Beschow (7):
>    docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
>    hw/block/pflash_cfi0{1,2}: Error out if device length isn't a power of
>      two
>    hw/sd/sdhci-internal: Unexport ESDHC defines
>    hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
>    hw/ppc/e500: Implement pflash handling
>    hw/sd/sdhci: Implement Freescale eSDHC device model
>    hw/ppc/e500: Add Freescale eSDHC to e500plat
> 
>   docs/system/ppc/ppce500.rst |  38 +++++++-
>   hw/block/pflash_cfi01.c     |   8 +-
>   hw/block/pflash_cfi02.c     |   5 +
>   hw/ppc/Kconfig              |   2 +
>   hw/ppc/e500.c               | 114 +++++++++++++++++++++-
>   hw/ppc/e500.h               |   1 +
>   hw/ppc/e500plat.c           |   1 +
>   hw/sd/sdhci-internal.h      |  20 ----
>   hw/sd/sdhci.c               | 183 +++++++++++++++++++++++++++++++-----
>   include/hw/sd/sdhci.h       |   3 +
>   10 files changed, 324 insertions(+), 51 deletions(-)
> 

Re: [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup

[B]

Am 26. Oktober 2022 17:18:14 UTC schrieb Daniel Henrique Barboza <danielhb413@gmail.com>:
>Hi,
>
>Since this is being sent to qemu-ppc and has to do with e500 I decided to
>take a look. I acked the e500 related patches, 5 and 7. Patch 6 LGTM as well
>but I'd rather not ack it it's SD specific code.
>
>I'll send a PowerPC pull request this week. I can grab this series via the ppc
>tree if someone with SD authority acks patch 6.

This would be awesome. If we can't reach consensus on the eSDHC device until then perhaps you could pull everything but the last two patches?

Thanks Daniel for generally absorbing any patches floating around which look remotely useful for the ppc tree. This is rewarding.

Best regards,
Bernhard
>
>
>Thanks,
>
>
>Daniel
>
>
>
>
>
>On 10/18/22 18:01, Bernhard Beschow wrote:
>> Cover letter:
>> ~~~~~~~~~~~~~
>> 
>> This series adds support for -pflash and direct SD card access to the
>> PPC e500 boards. The idea is to increase compatibility with "real" firmware
>> images where only the bare minimum of drivers is compiled in.
>> 
>> The series is structured as follows:
>> 
>> Patches 1-4 perform some general cleanup which paves the way for the rest of
>> the series.
>> 
>> Patch 5 adds -pflash handling where memory-mapped flash can be added on
>> user's behalf. That is, the flash memory region in the eLBC is only added if
>> the -pflash argument is supplied. Note that the cfi01 device model becomes
>> stricter in checking the size of the emulated flash space.
>> 
>> Patches 6 and 7 add a new device model - the Freescale eSDHC - to the e500
>> boards which was missing so far.
>> 
>> User documentation is also added as the new features become available.
>> 
>> Tesing done:
>> * `qeu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
>> "console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive
>> if=pflash,file=rootfs.ext2,format=raw`
>> * `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
>> "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive
>> id=mydrive,if=none,file=rootfs.ext2,format=raw`
>> 
>> The load was created using latest Buildroot with `make
>> qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type.
>> In both cases it was possible to log in and explore the root file system.
>> 
>> v4:
>> ~~~
>> Zoltan:
>> - Don't suggest presence of qemu-system-ppc32 in documentation
>> 
>> Bin:
>> - New patch: docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
>> 
>> Peter:
>> - Inline pflash_cfi01_register() rather than modify it (similar to v2)
>> 
>> v3:
>> ~~~
>> Phil:
>> - Also add power-of-2 fix to pflash_cfi02
>> - Resolve cfi01-specific assertion in e500 code
>> - Resolve unused define in eSDHC device model
>> - Resolve redundant alignment checks in eSDHC device model
>> 
>> Bin:
>> - Add dedicated flash chapter to documentation
>> 
>> Bernhard:
>> - Use is_power_of_2() instead of ctpop64() for better readability
>> - Only instantiate eSDHC device model in ppce500 (not used in MPC8544DS)
>> - Rebase onto gitlab.com/danielhb/qemu/tree/ppc-next
>> 
>> v2:
>> ~~~
>> Bin:
>> - Add source for MPC8544DS platform bus' memory map in commit message.
>> - Keep "ESDHC" in comment referring to Linux driver.
>> - Use "qemu-system-ppc{64|32} in documentation.
>> - Use g_autofree in device tree code.
>> - Remove unneeded device tree properties.
>> - Error out if pflash size doesn't fit into eLBC memory window.
>> - Remove unused ESDHC defines.
>> - Define macro ESDHC_WML for register offset with magic constant.
>> - Fix some whitespace issues when adding eSDHC device to e500.
>> 
>> Phil:
>> - Fix tense in commit message.
>> 
>> Bernhard Beschow (7):
>>    docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
>>    hw/block/pflash_cfi0{1,2}: Error out if device length isn't a power of
>>      two
>>    hw/sd/sdhci-internal: Unexport ESDHC defines
>>    hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
>>    hw/ppc/e500: Implement pflash handling
>>    hw/sd/sdhci: Implement Freescale eSDHC device model
>>    hw/ppc/e500: Add Freescale eSDHC to e500plat
>> 
>>   docs/system/ppc/ppce500.rst |  38 +++++++-
>>   hw/block/pflash_cfi01.c     |   8 +-
>>   hw/block/pflash_cfi02.c     |   5 +
>>   hw/ppc/Kconfig              |   2 +
>>   hw/ppc/e500.c               | 114 +++++++++++++++++++++-
>>   hw/ppc/e500.h               |   1 +
>>   hw/ppc/e500plat.c           |   1 +
>>   hw/sd/sdhci-internal.h      |  20 ----
>>   hw/sd/sdhci.c               | 183 +++++++++++++++++++++++++++++++-----
>>   include/hw/sd/sdhci.h       |   3 +
>>   10 files changed, 324 insertions(+), 51 deletions(-)
>> 

Re: [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup

[Daniel Henrique Barboza]

On 10/26/22 16:51, B wrote:
> 
> 
> Am 26. Oktober 2022 17:18:14 UTC schrieb Daniel Henrique Barboza <danielhb413@gmail.com>:
>> Hi,
>>
>> Since this is being sent to qemu-ppc and has to do with e500 I decided to
>> take a look. I acked the e500 related patches, 5 and 7. Patch 6 LGTM as well
>> but I'd rather not ack it it's SD specific code.
>>
>> I'll send a PowerPC pull request this week. I can grab this series via the ppc
>> tree if someone with SD authority acks patch 6.
> 
> This would be awesome. If we can't reach consensus on the eSDHC device until then perhaps you could pull everything but the last two patches?


That's fair enough. Just applied 1-5 to ppc-next.

I'll send a PR most likely Friday. If patch 6 gets an ACK until then I'll
pick 6 and 7 as well.

I'll be offline start of next week so this will be my last PR before the freeze.
If a patch 6 ACK arrives after Friday we'll need the SD maintainers to pick those
in before the freeze. Patch 7 has my ACK so feel free to take it.

> 
> Thanks Daniel for generally absorbing any patches floating around which look remotely useful for the ppc tree. This is rewarding.


Glad I'm able to help!


Daniel

> 
> Best regards,
> Bernhard
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>
>>
>>
>> On 10/18/22 18:01, Bernhard Beschow wrote:
>>> Cover letter:
>>> ~~~~~~~~~~~~~
>>>
>>> This series adds support for -pflash and direct SD card access to the
>>> PPC e500 boards. The idea is to increase compatibility with "real" firmware
>>> images where only the bare minimum of drivers is compiled in.
>>>
>>> The series is structured as follows:
>>>
>>> Patches 1-4 perform some general cleanup which paves the way for the rest of
>>> the series.
>>>
>>> Patch 5 adds -pflash handling where memory-mapped flash can be added on
>>> user's behalf. That is, the flash memory region in the eLBC is only added if
>>> the -pflash argument is supplied. Note that the cfi01 device model becomes
>>> stricter in checking the size of the emulated flash space.
>>>
>>> Patches 6 and 7 add a new device model - the Freescale eSDHC - to the e500
>>> boards which was missing so far.
>>>
>>> User documentation is also added as the new features become available.
>>>
>>> Tesing done:
>>> * `qeu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
>>> "console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive
>>> if=pflash,file=rootfs.ext2,format=raw`
>>> * `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
>>> "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive
>>> id=mydrive,if=none,file=rootfs.ext2,format=raw`
>>>
>>> The load was created using latest Buildroot with `make
>>> qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type.
>>> In both cases it was possible to log in and explore the root file system.
>>>
>>> v4:
>>> ~~~
>>> Zoltan:
>>> - Don't suggest presence of qemu-system-ppc32 in documentation
>>>
>>> Bin:
>>> - New patch: docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
>>>
>>> Peter:
>>> - Inline pflash_cfi01_register() rather than modify it (similar to v2)
>>>
>>> v3:
>>> ~~~
>>> Phil:
>>> - Also add power-of-2 fix to pflash_cfi02
>>> - Resolve cfi01-specific assertion in e500 code
>>> - Resolve unused define in eSDHC device model
>>> - Resolve redundant alignment checks in eSDHC device model
>>>
>>> Bin:
>>> - Add dedicated flash chapter to documentation
>>>
>>> Bernhard:
>>> - Use is_power_of_2() instead of ctpop64() for better readability
>>> - Only instantiate eSDHC device model in ppce500 (not used in MPC8544DS)
>>> - Rebase onto gitlab.com/danielhb/qemu/tree/ppc-next
>>>
>>> v2:
>>> ~~~
>>> Bin:
>>> - Add source for MPC8544DS platform bus' memory map in commit message.
>>> - Keep "ESDHC" in comment referring to Linux driver.
>>> - Use "qemu-system-ppc{64|32} in documentation.
>>> - Use g_autofree in device tree code.
>>> - Remove unneeded device tree properties.
>>> - Error out if pflash size doesn't fit into eLBC memory window.
>>> - Remove unused ESDHC defines.
>>> - Define macro ESDHC_WML for register offset with magic constant.
>>> - Fix some whitespace issues when adding eSDHC device to e500.
>>>
>>> Phil:
>>> - Fix tense in commit message.
>>>
>>> Bernhard Beschow (7):
>>>     docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
>>>     hw/block/pflash_cfi0{1,2}: Error out if device length isn't a power of
>>>       two
>>>     hw/sd/sdhci-internal: Unexport ESDHC defines
>>>     hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
>>>     hw/ppc/e500: Implement pflash handling
>>>     hw/sd/sdhci: Implement Freescale eSDHC device model
>>>     hw/ppc/e500: Add Freescale eSDHC to e500plat
>>>
>>>    docs/system/ppc/ppce500.rst |  38 +++++++-
>>>    hw/block/pflash_cfi01.c     |   8 +-
>>>    hw/block/pflash_cfi02.c     |   5 +
>>>    hw/ppc/Kconfig              |   2 +
>>>    hw/ppc/e500.c               | 114 +++++++++++++++++++++-
>>>    hw/ppc/e500.h               |   1 +
>>>    hw/ppc/e500plat.c           |   1 +
>>>    hw/sd/sdhci-internal.h      |  20 ----
>>>    hw/sd/sdhci.c               | 183 +++++++++++++++++++++++++++++++-----
>>>    include/hw/sd/sdhci.h       |   3 +
>>>    10 files changed, 324 insertions(+), 51 deletions(-)
>>>

Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[Philippe Mathieu-Daudé]

On 18/10/22 23:01, Bernhard Beschow wrote:
> Allows e500 boards to have their root file system reside on flash using
> only builtin devices located in the eLBC memory region.
> 
> Note that the flash memory area is only created when a -pflash argument is
> given, and that the size is determined by the given file. The idea is to
> put users into control.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   docs/system/ppc/ppce500.rst | 16 ++++++++
>   hw/ppc/Kconfig              |  1 +
>   hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 96 insertions(+)

> @@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
>                                   pmc->platform_bus_base,
>                                   &pms->pbus_dev->mmio);
>   
> +    dinfo = drive_get(IF_PFLASH, 0, 0);
> +    if (dinfo) {
> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> +        BlockDriverState *bs = blk_bs(blk);
> +        uint64_t size = bdrv_getlength(bs);
> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;
> +        uint32_t sector_len = 64 * KiB;
> +
> +        if (!is_power_of_2(size)) {
> +            error_report("Size of pflash file must be a power of two.");
> +            exit(1);
> +        }
> +
> +        if (size > mmio_size) {
> +            error_report("Size of pflash file must not be bigger than %" PRIu64
> +                         " bytes.", mmio_size);
> +            exit(1);
> +        }
> +
> +        if (!QEMU_IS_ALIGNED(size, sector_len)) {
> +            error_report("Size of pflash file must be a multiple of %" PRIu32
> +                         ".", sector_len);
> +            exit(1);
> +        }

Again, this check is unrelated to the board code and belong to the flash 
device (the board has no idea of the underlying flash restrictions).

(see below)

> +        dev = qdev_new(TYPE_PFLASH_CFI01);
> +        qdev_prop_set_drive(dev, "drive", blk);
> +        qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
> +        qdev_prop_set_uint64(dev, "sector-length", sector_len);
> +        qdev_prop_set_uint8(dev, "width", 2);
> +        qdev_prop_set_bit(dev, "big-endian", true);
> +        qdev_prop_set_uint16(dev, "id0", 0x89);
> +        qdev_prop_set_uint16(dev, "id1", 0x18);
> +        qdev_prop_set_uint16(dev, "id2", 0x0000);
> +        qdev_prop_set_uint16(dev, "id3", 0x0);
> +        qdev_prop_set_string(dev, "name", "e500.flash");
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

If you want to report the error differently, you can use a local Error*
and display it with error_report_err() before exiting.

Anyhow can be cleaned later, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +        memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
> +                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
> +    }

Re: [PATCH v4 7/7] hw/ppc/e500: Add Freescale eSDHC to e500plat

[Philippe Mathieu-Daudé]

On 18/10/22 23:01, Bernhard Beschow wrote:
> Adds missing functionality to e500plat machine which increases the
> chance of given "real" firmware images to access SD cards.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   docs/system/ppc/ppce500.rst | 12 ++++++++++++
>   hw/ppc/Kconfig              |  1 +
>   hw/ppc/e500.c               | 35 ++++++++++++++++++++++++++++++++++-
>   hw/ppc/e500.h               |  1 +
>   hw/ppc/e500plat.c           |  1 +
>   5 files changed, 49 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Philippe Mathieu-Daudé]

Hi Bernhard,

On 18/10/22 23:01, Bernhard Beschow wrote:
> Will allow e500 boards to access SD cards using just their own devices.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/sd/sdhci.c         | 120 +++++++++++++++++++++++++++++++++++++++++-
>   include/hw/sd/sdhci.h |   3 ++
>   2 files changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 306070c872..8d8ad9ff24 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
>       s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>   
>       s->io_ops = &sdhci_mmio_ops;
> +    s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
>   }
>   
>   void sdhci_uninitfn(SDHCIState *s)
> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>       s->fifo_buffer = g_malloc0(s->buf_maxsz);
>   
>       memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
> -                          SDHC_REGISTERS_MAP_SIZE);
> +                          s->io_registers_map_size);

I don't think we want to change this region size. [see below]

>   void sdhci_common_unrealize(SDHCIState *s)
> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = {
>       .class_init = sdhci_bus_class_init,
>   };
>   
> +/* --- qdev Freescale eSDHC --- */
> +
> +/* Watermark Level Register */
> +#define ESDHC_WML                    0x44
> +
> +/* Control Register for DMA transfer */
> +#define ESDHC_DMA_SYSCTL            0x40c
> +
> +#define ESDHC_REGISTERS_MAP_SIZE    0x410

My preferred approach would be to create a container region with a
size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region
in the container at offset 0, priority -1. Add 2 register regions
for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in
the container. ...

> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    uint64_t ret;
> +
> +    switch (offset) {
> +    case SDHC_SYSAD:
> +    case SDHC_BLKSIZE:
> +    case SDHC_ARGUMENT:
> +    case SDHC_TRNMOD:
> +    case SDHC_RSPREG0:
> +    case SDHC_RSPREG1:
> +    case SDHC_RSPREG2:
> +    case SDHC_RSPREG3:
> +    case SDHC_BDATA:
> +    case SDHC_PRNSTS:
> +    case SDHC_HOSTCTL:
> +    case SDHC_CLKCON:
> +    case SDHC_NORINTSTS:
> +    case SDHC_NORINTSTSEN:
> +    case SDHC_NORINTSIGEN:
> +    case SDHC_ACMD12ERRSTS:
> +    case SDHC_CAPAB:
> +    case SDHC_SLOT_INT_STATUS:
> +        ret = sdhci_read(opaque, offset, size);
> +        break;

... Then you don't need these cases.

> +    case ESDHC_WML:
> +    case ESDHC_DMA_SYSCTL:
> +        ret = 0;
> +        qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx
> +                      " not implemented\n", offset);

But then I realize you only treat these 2 registers as UNIMP.

So now, I'd create 1 UNIMP region for ESDHC_WML and map it
into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - 
SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
hw/arm/bcm2835_peripherals.c.

But the ESDHC_WML register has address 0x44 and fits inside the
SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
upper part of the SDHC_CAPAB register. These bits are undefined
on the spec v2, which I see you are setting in esdhci_init().
So this register should already return 0, otherwise we have
a bug. Thus we don't need to handle this ESDHC_WML particularly.

And your model is reduced to handling create_unimp() in esdhci_realize().

Am I missing something?

Regards,

Phil.

Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[Daniel Henrique Barboza]

Bernhard,

The 32 builds aren't fancying this patch. The issue is down there:

On 10/18/22 18:01, Bernhard Beschow wrote:
> Allows e500 boards to have their root file system reside on flash using
> only builtin devices located in the eLBC memory region.
> 
> Note that the flash memory area is only created when a -pflash argument is
> given, and that the size is determined by the given file. The idea is to
> put users into control.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   docs/system/ppc/ppce500.rst | 16 ++++++++
>   hw/ppc/Kconfig              |  1 +
>   hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 96 insertions(+)
> 
> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> index 7b5eb3c4ee..38f8ceb0cf 100644
> --- a/docs/system/ppc/ppce500.rst
> +++ b/docs/system/ppc/ppce500.rst
> @@ -165,3 +165,19 @@ if “-device eTSEC” is given to QEMU:
>   .. code-block:: bash
>   
>     -netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device eTSEC,netdev=net0
> +
> +Root file system on flash drive
> +-------------------------------
> +
> +Rather than using a root file system on ram disk, it is possible to have it on
> +CFI flash. Given an ext2 image whose size must be a power of two, it can be used
> +as follows:
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
> +      -display none -serial stdio \
> +      -kernel vmlinux \
> +      -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
> +      -append "rootwait root=/dev/mtdblock0"
> +
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 791fe78a50..769a1ead1c 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -126,6 +126,7 @@ config E500
>       select ETSEC
>       select GPIO_MPC8XXX
>       select OPENPIC
> +    select PFLASH_CFI01
>       select PLATFORM_BUS
>       select PPCE500_PCI
>       select SERIAL
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 3e950ea3ba..73198adac8 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -23,8 +23,10 @@
>   #include "e500-ccsr.h"
>   #include "net/net.h"
>   #include "qemu/config-file.h"
> +#include "hw/block/flash.h"
>   #include "hw/char/serial.h"
>   #include "hw/pci/pci.h"
> +#include "sysemu/block-backend-io.h"
>   #include "sysemu/sysemu.h"
>   #include "sysemu/kvm.h"
>   #include "sysemu/reset.h"
> @@ -267,6 +269,31 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
>       }
>   }
>   
> +static void create_devtree_flash(SysBusDevice *sbdev,
> +                                 PlatformDevtreeData *data)
> +{
> +    g_autofree char *name = NULL;
> +    uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
> +                                                   "num-blocks",
> +                                                   &error_fatal);
> +    uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
> +                                                      "sector-length",
> +                                                      &error_fatal);
> +    uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
> +                                                   "width",
> +                                                   &error_fatal);
> +    hwaddr flashbase = 0;
> +    hwaddr flashsize = num_blocks * sector_length;
> +    void *fdt = data->fdt;
> +
> +    name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
> +    qemu_fdt_add_subnode(fdt, name);
> +    qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
> +    qemu_fdt_setprop_sized_cells(fdt, name, "reg",
> +                                 1, flashbase, 1, flashsize);
> +    qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
> +}
> +
>   static void platform_bus_create_devtree(PPCE500MachineState *pms,
>                                           void *fdt, const char *mpic)
>   {
> @@ -276,6 +303,8 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
>       uint64_t addr = pmc->platform_bus_base;
>       uint64_t size = pmc->platform_bus_size;
>       int irq_start = pmc->platform_bus_first_irq;
> +    SysBusDevice *sbdev;
> +    bool ambiguous;
>   
>       /* Create a /platform node that we can put all devices into */
>   
> @@ -302,6 +331,13 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
>       /* Loop through all dynamic sysbus devices and create nodes for them */
>       foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
>   
> +    sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
> +                                                    &ambiguous));
> +    if (sbdev) {
> +        assert(!ambiguous);
> +        create_devtree_flash(sbdev, &data);
> +    }
> +
>       g_free(node);
>   }
>   
> @@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
>       unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
>       IrqLines *irqs;
>       DeviceState *dev, *mpicdev;
> +    DriveInfo *dinfo;
>       CPUPPCState *firstenv = NULL;
>       MemoryRegion *ccsr_addr_space;
>       SysBusDevice *s;
> @@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
>                                   pmc->platform_bus_base,
>                                   &pms->pbus_dev->mmio);
>   
> +    dinfo = drive_get(IF_PFLASH, 0, 0);
> +    if (dinfo) {
> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> +        BlockDriverState *bs = blk_bs(blk);
> +        uint64_t size = bdrv_getlength(bs);
> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;

^ here. The issue is that on a 32 bit system it is not possible to cast the
Int128 type to uint64_t:

FAILED: libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o
3746cc -m32 -Ilibqemu-ppc64-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../dtc/libfdt -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/danielhb/qemu -iquote /builds/danielhb/qemu/include -iquote /builds/danielhb/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc64-softmmu-config-devices.h"' -MD -MQ libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -MF libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o.d -o libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -c ../hw/ppc/e500.c
3747../hw/ppc/e500.c: In function 'ppce500_init':
3748../hw/ppc/e500.c:1069:30: error: incompatible types when initializing type 'uint64_t' {aka 'long long unsigned int'} using type 'Int128'
3749 1069 |         uint64_t mmio_size = pms->pbus_dev->mmio.size;
3750      |                              ^~~
3751[3207/5331] Compiling C object libqemu-ppc64-softmmu.fa.p/hw_ppc_mpc8544_guts.c.o


What I did to solve the problem is this:


+         uint64_t mmio_size = int128_get64(pms->pbus_dev->mmio.size);


This will get the lower 64 bits and return an uint64_t.

Note that this function will assert if mmio.size is bigger than UINT64_MAX, but
since you're doing an error(1) on the "if size > mmio_size" conditional, this
assert() is not introducing a new side effect. We'll just fail earlier with
a different error message.


Let me know if this is acceptable for you.


Daniel



> +        uint32_t sector_len = 64 * KiB;
> +
> +        if (!is_power_of_2(size)) {
> +            error_report("Size of pflash file must be a power of two.");
> +            exit(1);
> +        }
> +
> +        if (size > mmio_size) {
> +            error_report("Size of pflash file must not be bigger than %" PRIu64
> +                         " bytes.", mmio_size);
> +            exit(1);
> +        }
> +
> +        if (!QEMU_IS_ALIGNED(size, sector_len)) {
> +            error_report("Size of pflash file must be a multiple of %" PRIu32
> +                         ".", sector_len);
> +            exit(1);
> +        }
> +
> +        dev = qdev_new(TYPE_PFLASH_CFI01);
> +        qdev_prop_set_drive(dev, "drive", blk);
> +        qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
> +        qdev_prop_set_uint64(dev, "sector-length", sector_len);
> +        qdev_prop_set_uint8(dev, "width", 2);
> +        qdev_prop_set_bit(dev, "big-endian", true);
> +        qdev_prop_set_uint16(dev, "id0", 0x89);
> +        qdev_prop_set_uint16(dev, "id1", 0x18);
> +        qdev_prop_set_uint16(dev, "id2", 0x0000);
> +        qdev_prop_set_uint16(dev, "id3", 0x0);
> +        qdev_prop_set_string(dev, "name", "e500.flash");
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +
> +        memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
> +                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
> +    }
> +
>       /*
>        * Smart firmware defaults ahead!
>        *

Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[B]

Am 28. Oktober 2022 15:09:50 UTC schrieb Daniel Henrique Barboza <danielhb413@gmail.com>:
>Bernhard,

Hi Daniel,

>
>The 32 builds aren't fancying this patch. The issue is down there:
>
>On 10/18/22 18:01, Bernhard Beschow wrote:
>> Allows e500 boards to have their root file system reside on flash using
>> only builtin devices located in the eLBC memory region.
>> 
>> Note that the flash memory area is only created when a -pflash argument is
>> given, and that the size is determined by the given file. The idea is to
>> put users into control.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   docs/system/ppc/ppce500.rst | 16 ++++++++
>>   hw/ppc/Kconfig              |  1 +
>>   hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 96 insertions(+)
>> 
>> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
>> index 7b5eb3c4ee..38f8ceb0cf 100644
>> --- a/docs/system/ppc/ppce500.rst
>> +++ b/docs/system/ppc/ppce500.rst
>> @@ -165,3 +165,19 @@ if “-device eTSEC” is given to QEMU:
>>   .. code-block:: bash
>>       -netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device eTSEC,netdev=net0
>> +
>> +Root file system on flash drive
>> +-------------------------------
>> +
>> +Rather than using a root file system on ram disk, it is possible to have it on
>> +CFI flash. Given an ext2 image whose size must be a power of two, it can be used
>> +as follows:
>> +
>> +.. code-block:: bash
>> +
>> +  $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
>> +      -display none -serial stdio \
>> +      -kernel vmlinux \
>> +      -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
>> +      -append "rootwait root=/dev/mtdblock0"
>> +
>> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
>> index 791fe78a50..769a1ead1c 100644
>> --- a/hw/ppc/Kconfig
>> +++ b/hw/ppc/Kconfig
>> @@ -126,6 +126,7 @@ config E500
>>       select ETSEC
>>       select GPIO_MPC8XXX
>>       select OPENPIC
>> +    select PFLASH_CFI01
>>       select PLATFORM_BUS
>>       select PPCE500_PCI
>>       select SERIAL
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 3e950ea3ba..73198adac8 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -23,8 +23,10 @@
>>   #include "e500-ccsr.h"
>>   #include "net/net.h"
>>   #include "qemu/config-file.h"
>> +#include "hw/block/flash.h"
>>   #include "hw/char/serial.h"
>>   #include "hw/pci/pci.h"
>> +#include "sysemu/block-backend-io.h"
>>   #include "sysemu/sysemu.h"
>>   #include "sysemu/kvm.h"
>>   #include "sysemu/reset.h"
>> @@ -267,6 +269,31 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
>>       }
>>   }
>>   +static void create_devtree_flash(SysBusDevice *sbdev,
>> +                                 PlatformDevtreeData *data)
>> +{
>> +    g_autofree char *name = NULL;
>> +    uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
>> +                                                   "num-blocks",
>> +                                                   &error_fatal);
>> +    uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
>> +                                                      "sector-length",
>> +                                                      &error_fatal);
>> +    uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
>> +                                                   "width",
>> +                                                   &error_fatal);
>> +    hwaddr flashbase = 0;
>> +    hwaddr flashsize = num_blocks * sector_length;
>> +    void *fdt = data->fdt;
>> +
>> +    name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
>> +    qemu_fdt_add_subnode(fdt, name);
>> +    qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
>> +    qemu_fdt_setprop_sized_cells(fdt, name, "reg",
>> +                                 1, flashbase, 1, flashsize);
>> +    qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
>> +}
>> +
>>   static void platform_bus_create_devtree(PPCE500MachineState *pms,
>>                                           void *fdt, const char *mpic)
>>   {
>> @@ -276,6 +303,8 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
>>       uint64_t addr = pmc->platform_bus_base;
>>       uint64_t size = pmc->platform_bus_size;
>>       int irq_start = pmc->platform_bus_first_irq;
>> +    SysBusDevice *sbdev;
>> +    bool ambiguous;
>>         /* Create a /platform node that we can put all devices into */
>>   @@ -302,6 +331,13 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
>>       /* Loop through all dynamic sysbus devices and create nodes for them */
>>       foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
>>   +    sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
>> +                                                    &ambiguous));
>> +    if (sbdev) {
>> +        assert(!ambiguous);
>> +        create_devtree_flash(sbdev, &data);
>> +    }
>> +
>>       g_free(node);
>>   }
>>   @@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
>>       unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
>>       IrqLines *irqs;
>>       DeviceState *dev, *mpicdev;
>> +    DriveInfo *dinfo;
>>       CPUPPCState *firstenv = NULL;
>>       MemoryRegion *ccsr_addr_space;
>>       SysBusDevice *s;
>> @@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
>>                                   pmc->platform_bus_base,
>>                                   &pms->pbus_dev->mmio);
>>   +    dinfo = drive_get(IF_PFLASH, 0, 0);
>> +    if (dinfo) {
>> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> +        BlockDriverState *bs = blk_bs(blk);
>> +        uint64_t size = bdrv_getlength(bs);
>> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;
>
>^ here. The issue is that on a 32 bit system it is not possible to cast the
>Int128 type to uint64_t:
>
>FAILED: libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o
>3746cc -m32 -Ilibqemu-ppc64-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../dtc/libfdt -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/danielhb/qemu -iquote /builds/danielhb/qemu/include -iquote /builds/danielhb/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc64-softmmu-config-devices.h"' -MD -MQ libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -MF libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o.d -o libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -c ../hw/ppc/e500.c
>3747../hw/ppc/e500.c: In function 'ppce500_init':
>3748../hw/ppc/e500.c:1069:30: error: incompatible types when initializing type 'uint64_t' {aka 'long long unsigned int'} using type 'Int128'
>3749 1069 |         uint64_t mmio_size = pms->pbus_dev->mmio.size;
>3750      |                              ^~~
>3751[3207/5331] Compiling C object libqemu-ppc64-softmmu.fa.p/hw_ppc_mpc8544_guts.c.o

Whoops.
>
>
>What I did to solve the problem is this:
>
>
>+         uint64_t mmio_size = int128_get64(pms->pbus_dev->mmio.size);
>
>
>This will get the lower 64 bits and return an uint64_t.
>
>Note that this function will assert if mmio.size is bigger than UINT64_MAX, but
>since you're doing an error(1) on the "if size > mmio_size" conditional, this
>assert() is not introducing a new side effect. We'll just fail earlier with
>a different error message.

Yes, sounds reasonable.
>
>
>Let me know if this is acceptable for you.

Yes, that's fine with me. Thanks for the fix!

Best regards,
Bernhard
>
>
>Daniel
>
>
>
>> +        uint32_t sector_len = 64 * KiB;
>> +
>> +        if (!is_power_of_2(size)) {
>> +            error_report("Size of pflash file must be a power of two.");
>> +            exit(1);
>> +        }
>> +
>> +        if (size > mmio_size) {
>> +            error_report("Size of pflash file must not be bigger than %" PRIu64
>> +                         " bytes.", mmio_size);
>> +            exit(1);
>> +        }
>> +
>> +        if (!QEMU_IS_ALIGNED(size, sector_len)) {
>> +            error_report("Size of pflash file must be a multiple of %" PRIu32
>> +                         ".", sector_len);
>> +            exit(1);
>> +        }
>> +
>> +        dev = qdev_new(TYPE_PFLASH_CFI01);
>> +        qdev_prop_set_drive(dev, "drive", blk);
>> +        qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
>> +        qdev_prop_set_uint64(dev, "sector-length", sector_len);
>> +        qdev_prop_set_uint8(dev, "width", 2);
>> +        qdev_prop_set_bit(dev, "big-endian", true);
>> +        qdev_prop_set_uint16(dev, "id0", 0x89);
>> +        qdev_prop_set_uint16(dev, "id1", 0x18);
>> +        qdev_prop_set_uint16(dev, "id2", 0x0000);
>> +        qdev_prop_set_uint16(dev, "id3", 0x0);
>> +        qdev_prop_set_string(dev, "name", "e500.flash");
>> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +
>> +        memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
>> +                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
>> +    }
>> +
>>       /*
>>        * Smart firmware defaults ahead!
>>        *

Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[Philippe Mathieu-Daudé]

On 28/10/22 17:09, Daniel Henrique Barboza wrote:
> Bernhard,
> 
> The 32 builds aren't fancying this patch. The issue is down there:
> 
> On 10/18/22 18:01, Bernhard Beschow wrote:
>> Allows e500 boards to have their root file system reside on flash using
>> only builtin devices located in the eLBC memory region.
>>
>> Note that the flash memory area is only created when a -pflash 
>> argument is
>> given, and that the size is determined by the given file. The idea is to
>> put users into control.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   docs/system/ppc/ppce500.rst | 16 ++++++++
>>   hw/ppc/Kconfig              |  1 +
>>   hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 96 insertions(+)

>> @@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
>>                                   pmc->platform_bus_base,
>>                                   &pms->pbus_dev->mmio);
>> +    dinfo = drive_get(IF_PFLASH, 0, 0);
>> +    if (dinfo) {
>> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> +        BlockDriverState *bs = blk_bs(blk);
>> +        uint64_t size = bdrv_getlength(bs);
>> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;
> 
> ^ here. The issue is that on a 32 bit system it is not possible to cast the
> Int128 type to uint64_t:
> 
> FAILED: libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o
> 3746cc -m32 -Ilibqemu-ppc64-softmmu.fa.p -I. -I.. -Itarget/ppc 
> -I../target/ppc -I../dtc/libfdt -Iqapi -Itrace -Iui -Iui/shader 
> -I/usr/include/pixman-1 -I/usr/include/glib-2.0 
> -I/usr/lib/glib-2.0/include -I/usr/include/sysprof-4 
> -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g 
> -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers 
> -iquote . -iquote /builds/danielhb/qemu -iquote 
> /builds/danielhb/qemu/include -iquote /builds/danielhb/qemu/tcg/i386 
> -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE 
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
> -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes 
> -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration 
> -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k 
> -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs 
> -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 
> -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi 
> -fstack-protector-strong -fPIE -isystem../linux-headers 
> -isystemlinux-headers -DNEED_CPU_H 
> '-DCONFIG_TARGET="ppc64-softmmu-config-target.h"' 
> '-DCONFIG_DEVICES="ppc64-softmmu-config-devices.h"' -MD -MQ 
> libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -MF 
> libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o.d -o 
> libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -c ../hw/ppc/e500.c
> 3747../hw/ppc/e500.c: In function 'ppce500_init':
> 3748../hw/ppc/e500.c:1069:30: error: incompatible types when 
> initializing type 'uint64_t' {aka 'long long unsigned int'} using type 
> 'Int128'
> 3749 1069 |         uint64_t mmio_size = pms->pbus_dev->mmio.size;
> 3750      |                              ^~~
> 3751[3207/5331] Compiling C object 
> libqemu-ppc64-softmmu.fa.p/hw_ppc_mpc8544_guts.c.o
> 
> 
> What I did to solve the problem is this:
> 
> 
> +         uint64_t mmio_size = int128_get64(pms->pbus_dev->mmio.size); >
> This will get the lower 64 bits and return an uint64_t.
> 
> Note that this function will assert if mmio.size is bigger than 
> UINT64_MAX, but
> since you're doing an error(1) on the "if size > mmio_size" conditional, 
> this
> assert() is not introducing a new side effect. We'll just fail earlier with
> a different error message.

Simply use:

   memory_region_size(pms->pbus_dev->mmio);

Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[Daniel Henrique Barboza]

On 10/28/22 19:42, Philippe Mathieu-Daudé wrote:
> On 28/10/22 17:09, Daniel Henrique Barboza wrote:
>> Bernhard,
>>
>> The 32 builds aren't fancying this patch. The issue is down there:
>>
>> On 10/18/22 18:01, Bernhard Beschow wrote:
>>> Allows e500 boards to have their root file system reside on flash using
>>> only builtin devices located in the eLBC memory region.
>>>
>>> Note that the flash memory area is only created when a -pflash argument is
>>> given, and that the size is determined by the given file. The idea is to
>>> put users into control.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   docs/system/ppc/ppce500.rst | 16 ++++++++
>>>   hw/ppc/Kconfig              |  1 +
>>>   hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 96 insertions(+)
> 
>>> @@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
>>>                                   pmc->platform_bus_base,
>>>                                   &pms->pbus_dev->mmio);
>>> +    dinfo = drive_get(IF_PFLASH, 0, 0);
>>> +    if (dinfo) {
>>> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>>> +        BlockDriverState *bs = blk_bs(blk);
>>> +        uint64_t size = bdrv_getlength(bs);
>>> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;
>>
>> ^ here. The issue is that on a 32 bit system it is not possible to cast the
>> Int128 type to uint64_t:
>>
>> FAILED: libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o
>> 3746cc -m32 -Ilibqemu-ppc64-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../dtc/libfdt -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/danielhb/qemu -iquote /builds/danielhb/qemu/include -iquote /builds/danielhb/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value 
>> -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc64-softmmu-config-devices.h"' -MD -MQ libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -MF libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o.d -o libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -c ../hw/ppc/e500.c
>> 3747../hw/ppc/e500.c: In function 'ppce500_init':
>> 3748../hw/ppc/e500.c:1069:30: error: incompatible types when initializing type 'uint64_t' {aka 'long long unsigned int'} using type 'Int128'
>> 3749 1069 |         uint64_t mmio_size = pms->pbus_dev->mmio.size;
>> 3750      |                              ^~~
>> 3751[3207/5331] Compiling C object libqemu-ppc64-softmmu.fa.p/hw_ppc_mpc8544_guts.c.o
>>
>>
>> What I did to solve the problem is this:
>>
>>
>> +         uint64_t mmio_size = int128_get64(pms->pbus_dev->mmio.size); >
>> This will get the lower 64 bits and return an uint64_t.
>>
>> Note that this function will assert if mmio.size is bigger than UINT64_MAX, but
>> since you're doing an error(1) on the "if size > mmio_size" conditional, this
>> assert() is not introducing a new side effect. We'll just fail earlier with
>> a different error message.
> 
> Simply use:
> 
>    memory_region_size(pms->pbus_dev->mmio);

Nice! I'll change it in-tree before re-sending the PR.


Daniel

> 

Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling

[Bernhard Beschow]

Am 29. Oktober 2022 09:29:33 UTC schrieb Daniel Henrique Barboza <danielhb413@gmail.com>:
>
>
>On 10/28/22 19:42, Philippe Mathieu-Daudé wrote:
>> On 28/10/22 17:09, Daniel Henrique Barboza wrote:
>>> Bernhard,
>>> 
>>> The 32 builds aren't fancying this patch. The issue is down there:
>>> 
>>> On 10/18/22 18:01, Bernhard Beschow wrote:
>>>> Allows e500 boards to have their root file system reside on flash using
>>>> only builtin devices located in the eLBC memory region.
>>>> 
>>>> Note that the flash memory area is only created when a -pflash argument is
>>>> given, and that the size is determined by the given file. The idea is to
>>>> put users into control.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>   docs/system/ppc/ppce500.rst | 16 ++++++++
>>>>   hw/ppc/Kconfig              |  1 +
>>>>   hw/ppc/e500.c               | 79 +++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 96 insertions(+)
>> 
>>>> @@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
>>>>                                   pmc->platform_bus_base,
>>>>                                   &pms->pbus_dev->mmio);
>>>> +    dinfo = drive_get(IF_PFLASH, 0, 0);
>>>> +    if (dinfo) {
>>>> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>>>> +        BlockDriverState *bs = blk_bs(blk);
>>>> +        uint64_t size = bdrv_getlength(bs);
>>>> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;
>>> 
>>> ^ here. The issue is that on a 32 bit system it is not possible to cast the
>>> Int128 type to uint64_t:
>>> 
>>> FAILED: libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o
>>> 3746cc -m32 -Ilibqemu-ppc64-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../dtc/libfdt -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/danielhb/qemu -iquote /builds/danielhb/qemu/include -iquote /builds/danielhb/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc64-softmmu-config-devices.h"' -MD -MQ libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -MF libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o.d -o libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -c ../hw/ppc/e500.c
>>> 3747../hw/ppc/e500.c: In function 'ppce500_init':
>>> 3748../hw/ppc/e500.c:1069:30: error: incompatible types when initializing type 'uint64_t' {aka 'long long unsigned int'} using type 'Int128'
>>> 3749 1069 |         uint64_t mmio_size = pms->pbus_dev->mmio.size;
>>> 3750      |                              ^~~
>>> 3751[3207/5331] Compiling C object libqemu-ppc64-softmmu.fa.p/hw_ppc_mpc8544_guts.c.o
>>> 
>>> 
>>> What I did to solve the problem is this:
>>> 
>>> 
>>> +         uint64_t mmio_size = int128_get64(pms->pbus_dev->mmio.size); >
>>> This will get the lower 64 bits and return an uint64_t.
>>> 
>>> Note that this function will assert if mmio.size is bigger than UINT64_MAX, but
>>> since you're doing an error(1) on the "if size > mmio_size" conditional, this
>>> assert() is not introducing a new side effect. We'll just fail earlier with
>>> a different error message.
>> 
>> Simply use:
>> 
>>    memory_region_size(pms->pbus_dev->mmio);
>
>Nice! I'll change it in-tree before re-sending the PR.

Thanks Daniel!

Bernhard
>
>
>Daniel
>
>> 

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Bernhard Beschow]

Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Hi Bernhard,
>
>On 18/10/22 23:01, Bernhard Beschow wrote:
>> Will allow e500 boards to access SD cards using just their own devices.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/sd/sdhci.c         | 120 +++++++++++++++++++++++++++++++++++++++++-
>>   include/hw/sd/sdhci.h |   3 ++
>>   2 files changed, 122 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 306070c872..8d8ad9ff24 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
>>       s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>>         s->io_ops = &sdhci_mmio_ops;
>> +    s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
>>   }
>>     void sdhci_uninitfn(SDHCIState *s)
>> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>       s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>         memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>> -                          SDHC_REGISTERS_MAP_SIZE);
>> +                          s->io_registers_map_size);
>
>I don't think we want to change this region size. [see below]
>
>>   void sdhci_common_unrealize(SDHCIState *s)
>> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = {
>>       .class_init = sdhci_bus_class_init,
>>   };
>>   +/* --- qdev Freescale eSDHC --- */
>> +
>> +/* Watermark Level Register */
>> +#define ESDHC_WML                    0x44
>> +
>> +/* Control Register for DMA transfer */
>> +#define ESDHC_DMA_SYSCTL            0x40c
>> +
>> +#define ESDHC_REGISTERS_MAP_SIZE    0x410
>
>My preferred approach would be to create a container region with a
>size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region
>in the container at offset 0, priority -1. Add 2 register regions
>for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in
>the container. ...
>
>> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> +    uint64_t ret;
>> +
>> +    switch (offset) {
>> +    case SDHC_SYSAD:
>> +    case SDHC_BLKSIZE:
>> +    case SDHC_ARGUMENT:
>> +    case SDHC_TRNMOD:
>> +    case SDHC_RSPREG0:
>> +    case SDHC_RSPREG1:
>> +    case SDHC_RSPREG2:
>> +    case SDHC_RSPREG3:
>> +    case SDHC_BDATA:
>> +    case SDHC_PRNSTS:
>> +    case SDHC_HOSTCTL:
>> +    case SDHC_CLKCON:
>> +    case SDHC_NORINTSTS:
>> +    case SDHC_NORINTSTSEN:
>> +    case SDHC_NORINTSIGEN:
>> +    case SDHC_ACMD12ERRSTS:
>> +    case SDHC_CAPAB:
>> +    case SDHC_SLOT_INT_STATUS:
>> +        ret = sdhci_read(opaque, offset, size);
>> +        break;
>
>... Then you don't need these cases.
>
>> +    case ESDHC_WML:
>> +    case ESDHC_DMA_SYSCTL:
>> +        ret = 0;
>> +        qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx
>> +                      " not implemented\n", offset);
>
>But then I realize you only treat these 2 registers as UNIMP.
>
>So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>hw/arm/bcm2835_peripherals.c.
>
>But the ESDHC_WML register has address 0x44 and fits inside the
>SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>upper part of the SDHC_CAPAB register. These bits are undefined
>on the spec v2, which I see you are setting in esdhci_init().
>So this register should already return 0, otherwise we have
>a bug. Thus we don't need to handle this ESDHC_WML particularly.
>
>And your model is reduced to handling create_unimp() in esdhci_realize().
>
>Am I missing something?

The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?

Best regards,
Bernhard
>
>Regards,
>
>Phil.

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Bernhard Beschow]

Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>Hi Bernhard,
>>
>>On 18/10/22 23:01, Bernhard Beschow wrote:
>>> Will allow e500 boards to access SD cards using just their own devices.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   hw/sd/sdhci.c         | 120 +++++++++++++++++++++++++++++++++++++++++-
>>>   include/hw/sd/sdhci.h |   3 ++
>>>   2 files changed, 122 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 306070c872..8d8ad9ff24 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
>>>       s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>>>         s->io_ops = &sdhci_mmio_ops;
>>> +    s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
>>>   }
>>>     void sdhci_uninitfn(SDHCIState *s)
>>> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>       s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>         memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>>> -                          SDHC_REGISTERS_MAP_SIZE);
>>> +                          s->io_registers_map_size);
>>
>>I don't think we want to change this region size. [see below]
>>
>>>   void sdhci_common_unrealize(SDHCIState *s)
>>> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = {
>>>       .class_init = sdhci_bus_class_init,
>>>   };
>>>   +/* --- qdev Freescale eSDHC --- */
>>> +
>>> +/* Watermark Level Register */
>>> +#define ESDHC_WML                    0x44
>>> +
>>> +/* Control Register for DMA transfer */
>>> +#define ESDHC_DMA_SYSCTL            0x40c
>>> +
>>> +#define ESDHC_REGISTERS_MAP_SIZE    0x410
>>
>>My preferred approach would be to create a container region with a
>>size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region
>>in the container at offset 0, priority -1. Add 2 register regions
>>for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in
>>the container. ...
>>
>>> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
>>> +{
>>> +    uint64_t ret;
>>> +
>>> +    switch (offset) {
>>> +    case SDHC_SYSAD:
>>> +    case SDHC_BLKSIZE:
>>> +    case SDHC_ARGUMENT:
>>> +    case SDHC_TRNMOD:
>>> +    case SDHC_RSPREG0:
>>> +    case SDHC_RSPREG1:
>>> +    case SDHC_RSPREG2:
>>> +    case SDHC_RSPREG3:
>>> +    case SDHC_BDATA:
>>> +    case SDHC_PRNSTS:
>>> +    case SDHC_HOSTCTL:
>>> +    case SDHC_CLKCON:
>>> +    case SDHC_NORINTSTS:
>>> +    case SDHC_NORINTSTSEN:
>>> +    case SDHC_NORINTSIGEN:
>>> +    case SDHC_ACMD12ERRSTS:
>>> +    case SDHC_CAPAB:
>>> +    case SDHC_SLOT_INT_STATUS:
>>> +        ret = sdhci_read(opaque, offset, size);
>>> +        break;
>>
>>... Then you don't need these cases.
>>
>>> +    case ESDHC_WML:
>>> +    case ESDHC_DMA_SYSCTL:
>>> +        ret = 0;
>>> +        qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx
>>> +                      " not implemented\n", offset);
>>
>>But then I realize you only treat these 2 registers as UNIMP.
>>
>>So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>>into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>>another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>>0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>>hw/arm/bcm2835_peripherals.c.
>>
>>But the ESDHC_WML register has address 0x44 and fits inside the
>>SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>>upper part of the SDHC_CAPAB register. These bits are undefined
>>on the spec v2, which I see you are setting in esdhci_init().
>>So this register should already return 0, otherwise we have
>>a bug. Thus we don't need to handle this ESDHC_WML particularly.

My idea here was to catch this unimplemented case in order to indicate this clearly to users. Perhaps it nudges somebody to provide a patch?

>>
>>And your model is reduced to handling create_unimp() in esdhci_realize().
>>
>>Am I missing something?
>
>The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?

All in all I currently don't have a better idea than keeping the custom i/o ops for the standard region and adding an additional unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate memory for it where I still need to figure out how not to leak it.

Best regards,
Bernhard
>
>Best regards,
>Bernhard
>>
>>Regards,
>>
>>Phil.
>

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Bernhard Beschow]

Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>Hi Bernhard,
>>>
>>>On 18/10/22 23:01, Bernhard Beschow wrote:
>>>> Will allow e500 boards to access SD cards using just their own devices.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>   hw/sd/sdhci.c         | 120 +++++++++++++++++++++++++++++++++++++++++-
>>>>   include/hw/sd/sdhci.h |   3 ++
>>>>   2 files changed, 122 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>> index 306070c872..8d8ad9ff24 100644
>>>> --- a/hw/sd/sdhci.c
>>>> +++ b/hw/sd/sdhci.c
>>>> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
>>>>       s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>>>>         s->io_ops = &sdhci_mmio_ops;
>>>> +    s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
>>>>   }
>>>>     void sdhci_uninitfn(SDHCIState *s)
>>>> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>>       s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>>         memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>>>> -                          SDHC_REGISTERS_MAP_SIZE);
>>>> +                          s->io_registers_map_size);
>>>
>>>I don't think we want to change this region size. [see below]
>>>
>>>>   void sdhci_common_unrealize(SDHCIState *s)
>>>> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = {
>>>>       .class_init = sdhci_bus_class_init,
>>>>   };
>>>>   +/* --- qdev Freescale eSDHC --- */
>>>> +
>>>> +/* Watermark Level Register */
>>>> +#define ESDHC_WML                    0x44
>>>> +
>>>> +/* Control Register for DMA transfer */
>>>> +#define ESDHC_DMA_SYSCTL            0x40c
>>>> +
>>>> +#define ESDHC_REGISTERS_MAP_SIZE    0x410
>>>
>>>My preferred approach would be to create a container region with a
>>>size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region
>>>in the container at offset 0, priority -1. Add 2 register regions
>>>for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in
>>>the container. ...
>>>
>>>> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
>>>> +{
>>>> +    uint64_t ret;
>>>> +
>>>> +    switch (offset) {
>>>> +    case SDHC_SYSAD:
>>>> +    case SDHC_BLKSIZE:
>>>> +    case SDHC_ARGUMENT:
>>>> +    case SDHC_TRNMOD:
>>>> +    case SDHC_RSPREG0:
>>>> +    case SDHC_RSPREG1:
>>>> +    case SDHC_RSPREG2:
>>>> +    case SDHC_RSPREG3:
>>>> +    case SDHC_BDATA:
>>>> +    case SDHC_PRNSTS:
>>>> +    case SDHC_HOSTCTL:
>>>> +    case SDHC_CLKCON:
>>>> +    case SDHC_NORINTSTS:
>>>> +    case SDHC_NORINTSTSEN:
>>>> +    case SDHC_NORINTSIGEN:
>>>> +    case SDHC_ACMD12ERRSTS:
>>>> +    case SDHC_CAPAB:
>>>> +    case SDHC_SLOT_INT_STATUS:
>>>> +        ret = sdhci_read(opaque, offset, size);
>>>> +        break;
>>>
>>>... Then you don't need these cases.
>>>
>>>> +    case ESDHC_WML:
>>>> +    case ESDHC_DMA_SYSCTL:
>>>> +        ret = 0;
>>>> +        qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx
>>>> +                      " not implemented\n", offset);
>>>
>>>But then I realize you only treat these 2 registers as UNIMP.
>>>
>>>So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>>>into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>>>another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>>>0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>>>hw/arm/bcm2835_peripherals.c.
>>>
>>>But the ESDHC_WML register has address 0x44 and fits inside the
>>>SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>>>upper part of the SDHC_CAPAB register. These bits are undefined
>>>on the spec v2, which I see you are setting in esdhci_init().
>>>So this register should already return 0, otherwise we have
>>>a bug. Thus we don't need to handle this ESDHC_WML particularly.
>
>My idea here was to catch this unimplemented case in order to indicate this clearly to users. Perhaps it nudges somebody to provide a patch?
>
>>>
>>>And your model is reduced to handling create_unimp() in esdhci_realize().
>>>
>>>Am I missing something?
>>
>>The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
>
>All in all I currently don't have a better idea than keeping the custom i/o ops for the standard region and adding an additional unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate memory for it where I still need to figure out how not to leak it.

By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I was able to remove the custom implementations while having big endian and the alignments proper. However, I don't see a way of adding two memory regions - with or without a container. With a container I'd have to somehow preserve the mmio attribute which is initialized by the parent class, re-initialize it with the container, and add the preserved memory region as child. This seems very fragile, esp. since the parent class has created an alias for mmio in sysbus. Without a container, one would have two memory regions that both have to be mapped separately by the caller, i.e. it burdens the caller with an implementation detail.

Any suggestions?

Best regards,
Bernhard
>
>Best regards,
>Bernhard
>>
>>Best regards,
>>Bernhard
>>>
>>>Regards,
>>>
>>>Phil.
>>
>

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Philippe Mathieu-Daudé]

On 29/10/22 20:28, Bernhard Beschow wrote:
> Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>> Hi Bernhard,
>>>>
>>>> On 18/10/22 23:01, Bernhard Beschow wrote:
>>>>> Will allow e500 boards to access SD cards using just their own devices.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>>    hw/sd/sdhci.c         | 120 +++++++++++++++++++++++++++++++++++++++++-
>>>>>    include/hw/sd/sdhci.h |   3 ++
>>>>>    2 files changed, 122 insertions(+), 1 deletion(-)

>>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>>>> hw/arm/bcm2835_peripherals.c.
>>>>
>>>> But the ESDHC_WML register has address 0x44 and fits inside the
>>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>>>> upper part of the SDHC_CAPAB register. These bits are undefined
>>>> on the spec v2, which I see you are setting in esdhci_init().
>>>> So this register should already return 0, otherwise we have
>>>> a bug. Thus we don't need to handle this ESDHC_WML particularly.
>>
>> My idea here was to catch this unimplemented case in order to indicate this clearly to users. Perhaps it nudges somebody to provide a patch?
>>
>>>>
>>>> And your model is reduced to handling create_unimp() in esdhci_realize().
>>>>
>>>> Am I missing something?
>>>
>>> The mmio ops are big endian and need to be aligned to a 4-byte boundary. It took me quite a while to debug this. So shall I just create an additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
>>
>> All in all I currently don't have a better idea than keeping the custom i/o ops for the standard region and adding an additional unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate memory for it where I still need to figure out how not to leak it.
> 
> By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I was able to remove the custom implementations while having big endian and the alignments proper. However, I don't see a way of adding two memory regions - with or without a container. With a container I'd have to somehow preserve the mmio attribute which is initialized by the parent class, re-initialize it with the container, and add the preserved memory region as child. This seems very fragile, esp. since the parent class has created an alias for mmio in sysbus. Without a container, one would have two memory regions that both have to be mapped separately by the caller, i.e. it burdens the caller with an implementation detail.
> 
> Any suggestions?

Can you share branch and how to test?

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Bernhard Beschow]

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

On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 29/10/22 20:28, Bernhard Beschow wrote:
> > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow <
> shentey@gmail.com>:
> >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow <
> shentey@gmail.com>:
> >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé" <
> philmd@linaro.org>:
> >>>> Hi Bernhard,
> >>>>
> >>>> On 18/10/22 23:01, Bernhard Beschow wrote:
> >>>>> Will allow e500 boards to access SD cards using just their own
> devices.
> >>>>>
> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >>>>> ---
> >>>>>    hw/sd/sdhci.c         | 120
> +++++++++++++++++++++++++++++++++++++++++-
> >>>>>    include/hw/sd/sdhci.h |   3 ++
> >>>>>    2 files changed, 122 insertions(+), 1 deletion(-)
>
> >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it
> >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
> >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE -
> SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
> >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
> >>>> hw/arm/bcm2835_peripherals.c.
> >>>>
> >>>> But the ESDHC_WML register has address 0x44 and fits inside the
> >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
> >>>> upper part of the SDHC_CAPAB register. These bits are undefined
> >>>> on the spec v2, which I see you are setting in esdhci_init().
> >>>> So this register should already return 0, otherwise we have
> >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly.
> >>
> >> My idea here was to catch this unimplemented case in order to indicate
> this clearly to users. Perhaps it nudges somebody to provide a patch?
> >>
> >>>>
> >>>> And your model is reduced to handling create_unimp() in
> esdhci_realize().
> >>>>
> >>>> Am I missing something?
> >>>
> >>> The mmio ops are big endian and need to be aligned to a 4-byte
> boundary. It took me quite a while to debug this. So shall I just create an
> additional memory region for the region above SDHC_REGISTERS_MAP_SIZE for
> ESDHC_DMA_SYSCTL?
> >>
> >> All in all I currently don't have a better idea than keeping the custom
> i/o ops for the standard region and adding an additional unimplemented
> region for ESDHC_DMA_SYSCTL. I think I'd have to dynamically allocate
> memory for it where I still need to figure out how not to leak it.
> >
> > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I was able
> to remove the custom implementations while having big endian and the
> alignments proper. However, I don't see a way of adding two memory regions
> - with or without a container. With a container I'd have to somehow
> preserve the mmio attribute which is initialized by the parent class,
> re-initialize it with the container, and add the preserved memory region as
> child. This seems very fragile, esp. since the parent class has created an
> alias for mmio in sysbus. Without a container, one would have two memory
> regions that both have to be mapped separately by the caller, i.e. it
> burdens the caller with an implementation detail.
> >
> > Any suggestions?
>
> Can you share branch and how to test?
>

QEMU branch: https://github.com/shentok/qemu/tree/e500-flash

How to test:
1. `git clone -b e500 https://github.com/shentok/buildroot.git`
2. `cd buildroot`
3. `make qemu_ppc_e500mc_defconfig`
4. `make`
5. `cd output/images`
6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2
of=root.img bs=1M conv=notrunc`
7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
"console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive
-drive id=mydrive,if=none,file=root.img,format=raw`

Note that step 6 might be required every time before qemu-system-ppc is
started, otherwise this may cause an fsck. The output on the boot console
will look something along these lines (note that no errors are reported):

  Memory CAM mapping: 16/16/16/16/64/64/64 Mb, residual: 0Mb
  Activating Kernel Userspace Access Protection
  Activating Kernel Userspace Execution Prevention
  Linux version 5.17.7 (zcone-pisint@osoxes)
(powerpc-buildroot-linux-gnu-gcc.br_real (Buildroot
2022.08-655-gf8a0d1480a) 11.3.0, GNU ld (GNU Binutils) 2.38) #1 SMP Sat Oct
29 12:49:54 CEST 2022
  Using QEMU e500 machine description
  printk: bootconsole [udbg0] enabled
  CPU maps initialized for 1 thread per core
  -----------------------------------------------------
  phys_mem_size     = 0x10000000
  dcache_bsize      = 0x40
  icache_bsize      = 0x40
  cpu_features      = 0x0000000000000194
    possible        = 0x000000000001039c
    always          = 0x0000000000000100
  cpu_user_features = 0x8c008000 0x08000000
  mmu_features      = 0x000a0010
  -----------------------------------------------------
  qemu_e500_setup_arch()
  barrier-nospec: using isync; sync as speculation barrier
  Zone ranges:
    Normal   [mem 0x0000000000000000-0x000000000fffffff]
    HighMem  empty
  Movable zone start for each node
  Early memory node ranges
    node   0: [mem 0x0000000000000000-0x000000000fffffff]
  Initmem setup node 0 [mem 0x0000000000000000-0x000000000fffffff]
  MMU: Allocated 1088 bytes of context maps for 255 contexts
  percpu: Embedded 16 pages/cpu s33804 r8192 d23540 u65536
  Built 1 zonelists, mobility grouping on.  Total pages: 65024
  Kernel command line: console=ttyS0 rootwait root=/dev/mmcblk0 nokaslr
  Unknown kernel command line parameters "nokaslr", will be passed to user
space.
  Dentry cache hash table entries: 32768 (order: 5, 131072 bytes, linear)
  Inode-cache hash table entries: 16384 (order: 4, 65536 bytes, linear)
  mem auto-init: stack:off, heap alloc:off, heap free:off
  Kernel virtual memory layout:
    * 0xffa5f000..0xfffff000  : fixmap
    * 0xff800000..0xffa00000  : highmem PTEs
    * 0xd1000000..0xff800000  : vmalloc & ioremap
  Memory: 174608K/262144K available (12680K kernel code, 1080K rwdata,
3552K rodata, 396K init, 238K bss, 87536K reserved, 0K cma-reserved, 0K
highmem)
  SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
  rcu: Hierarchical RCU implementation.
  rcu: RCU event tracing is enabled.
  rcu: RCU restricting CPUs from NR_CPUS=24 to nr_cpu_ids=1.
  rcu: RCU calculated value of scheduler-enlistment delay is 25 jiffies.
  rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1
  NR_IRQS: 512, nr_irqs: 512, preallocated irqs: 16
  mpic: Setting up MPIC " OpenPIC  " version 1.2 at fe0040000, max 1 CPUs
  mpic: ISU size: 256, shift: 8, mask: ff
  mpic: Initializing for 256 sources
  random: get_random_u32 called from start_kernel+0x524/0x6b0 with
crng_init=0
  clocksource: timebase: mask: 0xffffffffffffffff max_cycles: 0x5c4093a7d1,
max_idle_ns: 440795210635 ns
  clocksource: timebase mult[2800000] shift[24] registered
  Console: colour dummy device 80x25
  pid_max: default: 32768 minimum: 301
  Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
  Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
  e500 family performance monitor hardware support registered
  rcu: Hierarchical SRCU implementation.
  smp: Bringing up secondary CPUs ...
  smp: Brought up 1 node, 1 CPU
  devtmpfs: initialized
  clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff,
max_idle_ns: 7645041785100000 ns
  futex hash table entries: 256 (order: 2, 16384 bytes, linear)
  NET: Registered PF_NETLINK/PF_ROUTE protocol family
  audit: initializing netlink subsys (disabled)

  Found FSL PCI host bridge at 0x0000000fe0008000. Firmware bus number:
0->255
  PCI host bridge /pci@fe0008000 (primary) ranges:
   MEM 0x0000000c00000000..0x0000000c1fffffff -> 0x00000000e0000000
    IO 0x0000000fe1000000..0x0000000fe100ffff -> 0x0000000000000000
  /pci@fe0008000: PCICSRBAR @ 0xdff00000
  setup_pci_atmu: end of DRAM 10000000
  Machine: QEMU ppce500
  SoC family: QorIQ
  SoC ID: svr:0x00000000, Revision: 0.0
  audit: type=2000 audit(0.092:1): state=initialized audit_enabled=0 res=1
  fsl-pamu: fsl_pamu_init: could not find a PAMU node
  software IO TLB: tearing down default memory pool
  PCI: Probing PCI hardware
  fsl-pci fe0008000.pci: PCI host bridge to bus 8000:00
  pci_bus 8000:00: root bus resource [io  0x0000-0xffff]
  pci_bus 8000:00: root bus resource [mem 0xc00000000-0xc1fffffff] (bus
address [0xe0000000-0xffffffff])
  pci_bus 8000:00: root bus resource [bus 00-ff]
  pci_bus 8000:00: busn_res: [bus 00-ff] end is updated to ff
  pci 8000:00:00.0: [1957:0030] type 00 class 0x0b2000
  pci 8000:00:00.0: reg 0x10: [mem 0xdff00000-0xdfffffff]
  pci 8000:00:01.0: [1af4:1000] type 00 class 0x020000
  pci 8000:00:01.0: reg 0x10: [io  0x0000-0x001f]
  pci 8000:00:01.0: reg 0x14: [mem 0x00000000-0x00000fff]
  pci 8000:00:01.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit pref]
  pci 8000:00:01.0: reg 0x30: [mem 0x00000000-0x0003ffff pref]
  pci_bus 8000:00: busn_res: [bus 00-ff] end is updated to 00
  pci 8000:00:01.0: BAR 6: assigned [mem 0xc00000000-0xc0003ffff pref]
  pci 8000:00:01.0: BAR 4: assigned [mem 0xc00040000-0xc00043fff 64bit pref]
  pci 8000:00:01.0: BAR 1: assigned [mem 0xc00044000-0xc00044fff]
  pci 8000:00:01.0: BAR 0: assigned [io  0x1000-0x101f]
  pci_bus 8000:00: resource 4 [io  0x0000-0xffff]
  pci_bus 8000:00: resource 5 [mem 0xc00000000-0xc1fffffff]
  HugeTLB registered 4.00 MiB page size, pre-allocated 0 pages
  HugeTLB registered 16.0 MiB page size, pre-allocated 0 pages
  HugeTLB registered 64.0 MiB page size, pre-allocated 0 pages
  HugeTLB registered 256 MiB page size, pre-allocated 0 pages
  HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
  Freescale Elo series DMA driver
  iommu: Default domain type: Translated
  iommu: DMA domain TLB invalidation policy: strict mode
  vgaarb: loaded
  SCSI subsystem initialized
  usbcore: registered new interface driver usbfs
  usbcore: registered new interface driver hub
  usbcore: registered new device driver usb
  pps_core: LinuxPPS API ver. 1 registered
  pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti <
giometti@linux.it>
  PTP clock support registered
  Advanced Linux Sound Architecture Driver Initialized.
  clocksource: Switched to clocksource timebase
  NET: Registered PF_INET protocol family
  IP idents hash table entries: 4096 (order: 3, 32768 bytes, linear)
  tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 6144 bytes,
linear)
  TCP established hash table entries: 2048 (order: 1, 8192 bytes, linear)
  TCP bind hash table entries: 2048 (order: 2, 16384 bytes, linear)
  TCP: Hash tables configured (established 2048 bind 2048)
  UDP hash table entries: 256 (order: 1, 8192 bytes, linear)
  UDP-Lite hash table entries: 256 (order: 1, 8192 bytes, linear)
  NET: Registered PF_UNIX/PF_LOCAL protocol family
  RPC: Registered named UNIX socket transport module.
  RPC: Registered udp transport module.
  RPC: Registered tcp transport module.
  RPC: Registered tcp NFSv4.1 backchannel transport module.
  PCI: CLS 0 bytes, default 64
  workingset: timestamp_bits=30 max_order=16 bucket_order=0
  squashfs: version 4.0 (2009/01/31) Phillip Lougher
  NFS: Registering the id_resolver key type
  Key type id_resolver registered
  Key type id_legacy registered
  Installing knfsd (copyright (C) 1996 okir@monad.swb.de).
  ntfs: driver 2.1.32 [Flags: R/O].
  jffs2: version 2.2. (NAND) © 2001-2006 Red Hat, Inc.
  io scheduler mq-deadline registered
  io scheduler kyber registered
  virtio-pci 8000:00:01.0: enabling device (0000 -> 0003)
  Serial: 8250/16550 driver, 6 ports, IRQ sharing enabled
  printk: console [ttyS0] disabled
  serial8250.0: ttyS0 at MMIO 0xfe0004500 (irq = 42, base_baud = 25000000)
is a 16550A
  printk: console [ttyS0] enabled
  printk: console [ttyS0] enabled
  printk: bootconsole [udbg0] disabled
  printk: bootconsole [udbg0] disabled
  ePAPR hypervisor byte channel driver
  brd: module loaded
  loop: module loaded
  st: Version 20160209, fixed bufsize 32768, s/g segs 256
  ucc_geth_driver: QE UCC Gigabit Ethernet Controller
  e1000: Intel(R) PRO/1000 Network Driver
  e1000: Copyright (c) 1999-2006 Intel Corporation.
  e1000e: Intel(R) PRO/1000 Network Driver
  e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
  igb: Intel(R) Gigabit Ethernet Network Driver
  igb: Copyright (c) 2007-2014 Intel Corporation.
  ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
  ehci-pci: EHCI PCI platform driver
  ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
  ohci-pci: OHCI PCI platform driver
  ehci-fsl: Freescale EHCI Host controller driver
  usbcore: registered new interface driver usb-storage
  i2c_dev: i2c /dev entries driver
  mpc-i2c fe0003000.i2c: timeout 1000000 us
  rtc-ds1307 0-0068: registered as rtc0
  rtc-ds1307 0-0068: setting system clock to 2022-10-30T11:27:44 UTC
(1667129264)
  sdhci: Secure Digital Host Controller Interface driver
  sdhci: Copyright(c) Pierre Ossman
  sdhci-pltfm: SDHCI platform and OF driver helper
  Freescale hypervisor management driver
  fsl-hv: no hypervisor found
  mmc0 bounce up to 128 segments into one, max segment size 65536 bytes
  ipip: IPv4 and MPLS over IPv4 tunneling driver
  Initializing XFRM netlink socket
  NET: Registered PF_INET6 protocol family
  Segment Routing with IPv6
  In-situ OAM (IOAM) with IPv6
  sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
  NET: Registered PF_PACKET protocol family
  NET: Registered PF_KEY protocol family
  Key type dns_resolver registered
  drmem: No dynamic reconfiguration memory found
  mmc0: SDHCI controller on fe002e000.sdhc [fe002e000.sdhc] using DMA
  ALSA device list:
    No soundcards found.
  Waiting for root device /dev/mmcblk0...
  mmc0: new high speed SD card at address 4567
  mmcblk0: mmc0:4567 QEMU! 64.0 MiB
  EXT4-fs (mmcblk0): mounted filesystem without journal. Quota mode:
disabled.
  VFS: Mounted root (ext4 filesystem) readonly on device 179:0.
  devtmpfs: mounted
  Freeing unused kernel image (initmem) memory: 396K
  Run /sbin/init as init process
  EXT4-fs (mmcblk0): re-mounted. Quota mode: disabled.
  Starting syslogd: OK
  Starting klogd: OK
  Running sysctl: OK
  Saving random seed: random: dd: uninitialized urandom read (512 bytes
read)
  OK
  Starting network: udhcpc: started, v1.35.0
  udhcpc: broadcasting discover
  udhcpc: broadcasting select for 10.0.2.15, server 10.0.2.2
  random: fast init done
  udhcpc: lease of 10.0.2.15 obtained from 10.0.2.2, lease time 86400
  deleting routers
  adding dns 10.0.2.3
  OK

  Welcome to Buildroot
  buildroot login:


Best regards,
Bernhard

[-- Attachment #2: Type: text/html, Size: 21064 bytes --]

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Philippe Mathieu-Daudé]

On 30/10/22 12:46, Bernhard Beschow wrote:
> 
> 
> On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé 
> <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
> 
>     On 29/10/22 20:28, Bernhard Beschow wrote:
>      > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow
>     <shentey@gmail.com <mailto:shentey@gmail.com>>:
>      >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow
>     <shentey@gmail.com <mailto:shentey@gmail.com>>:
>      >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe
>     Mathieu-Daudé" <philmd@linaro.org <mailto:philmd@linaro.org>>:
>      >>>> Hi Bernhard,
>      >>>>
>      >>>> On 18/10/22 23:01, Bernhard Beschow wrote:
>      >>>>> Will allow e500 boards to access SD cards using just their
>     own devices.
>      >>>>>
>      >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com
>     <mailto:shentey@gmail.com>>
>      >>>>> ---
>      >>>>>    hw/sd/sdhci.c         | 120
>     +++++++++++++++++++++++++++++++++++++++++-
>      >>>>>    include/hw/sd/sdhci.h |   3 ++
>      >>>>>    2 files changed, 122 insertions(+), 1 deletion(-)
> 
>      >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>      >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>      >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE -
>     SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>      >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>      >>>> hw/arm/bcm2835_peripherals.c.
>      >>>>
>      >>>> But the ESDHC_WML register has address 0x44 and fits inside the
>      >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>      >>>> upper part of the SDHC_CAPAB register. These bits are undefined
>      >>>> on the spec v2, which I see you are setting in esdhci_init().
>      >>>> So this register should already return 0, otherwise we have
>      >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly.
>      >>
>      >> My idea here was to catch this unimplemented case in order to
>     indicate this clearly to users. Perhaps it nudges somebody to
>     provide a patch?
>      >>
>      >>>>
>      >>>> And your model is reduced to handling create_unimp() in
>     esdhci_realize().
>      >>>>
>      >>>> Am I missing something?
>      >>>
>      >>> The mmio ops are big endian and need to be aligned to a 4-byte
>     boundary. It took me quite a while to debug this. So shall I just
>     create an additional memory region for the region above
>     SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
>      >>
>      >> All in all I currently don't have a better idea than keeping the
>     custom i/o ops for the standard region and adding an additional
>     unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to
>     dynamically allocate memory for it where I still need to figure out
>     how not to leak it.
>      >
>      > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I
>     was able to remove the custom implementations while having big
>     endian and the alignments proper. However, I don't see a way of
>     adding two memory regions - with or without a container. With a
>     container I'd have to somehow preserve the mmio attribute which is
>     initialized by the parent class, re-initialize it with the
>     container, and add the preserved memory region as child. This seems
>     very fragile, esp. since the parent class has created an alias for
>     mmio in sysbus. Without a container, one would have two memory
>     regions that both have to be mapped separately by the caller, i.e.
>     it burdens the caller with an implementation detail.
>      >
>      > Any suggestions?

See 
https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/

>     Can you share branch and how to test?
> 
> 
> QEMU branch: https://github.com/shentok/qemu/tree/e500-flash 
> <https://github.com/shentok/qemu/tree/e500-flash>
> 
> How to test:
> 1. `git clone -b e500 https://github.com/shentok/buildroot.git 
> <https://github.com/shentok/buildroot.git>`
> 2. `cd buildroot`
> 3. `make qemu_ppc_e500mc_defconfig`
> 4. `make`
> 5. `cd output/images`
> 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 
> of=root.img bs=1M conv=notrunc`
> 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append 
> "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive 
> -drive id=mydrive,if=none,file=root.img,format=raw`

Could you add an Avocado-based test?

>    Welcome to Buildroot
>    buildroot login:

Regards,

Phil.

Re: [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup

[Philippe Mathieu-Daudé]

Hi Daniel,

On 26/10/22 19:18, Daniel Henrique Barboza wrote:
> Hi,
> 
> Since this is being sent to qemu-ppc and has to do with e500 I decided to
> take a look. I acked the e500 related patches, 5 and 7. Patch 6 LGTM as 
> well
> but I'd rather not ack it it's SD specific code.
> 
> I'll send a PowerPC pull request this week. I can grab this series via 
> the ppc
> tree if someone with SD authority acks patch 6.

I was not very happy with the SDHCI changes and sent a counter (simpler)
proposal to Bernhard, see:
https://lore.kernel.org/qemu-devel/20221031115402.91912-1-philmd@linaro.org/

Regards,

Phil.

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Bernhard Beschow]

Am 31. Oktober 2022 12:11:37 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 30/10/22 12:46, Bernhard Beschow wrote:
>> 
>> 
>> On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
>> 
>>     On 29/10/22 20:28, Bernhard Beschow wrote:
>>      > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow
>>     <shentey@gmail.com <mailto:shentey@gmail.com>>:
>>      >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow
>>     <shentey@gmail.com <mailto:shentey@gmail.com>>:
>>      >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe
>>     Mathieu-Daudé" <philmd@linaro.org <mailto:philmd@linaro.org>>:
>>      >>>> Hi Bernhard,
>>      >>>>
>>      >>>> On 18/10/22 23:01, Bernhard Beschow wrote:
>>      >>>>> Will allow e500 boards to access SD cards using just their
>>     own devices.
>>      >>>>>
>>      >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com
>>     <mailto:shentey@gmail.com>>
>>      >>>>> ---
>>      >>>>>    hw/sd/sdhci.c         | 120
>>     +++++++++++++++++++++++++++++++++++++++++-
>>      >>>>>    include/hw/sd/sdhci.h |   3 ++
>>      >>>>>    2 files changed, 122 insertions(+), 1 deletion(-)
>> 
>>      >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>>      >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>>      >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE -
>>     SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>>      >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>>      >>>> hw/arm/bcm2835_peripherals.c.
>>      >>>>
>>      >>>> But the ESDHC_WML register has address 0x44 and fits inside the
>>      >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>>      >>>> upper part of the SDHC_CAPAB register. These bits are undefined
>>      >>>> on the spec v2, which I see you are setting in esdhci_init().
>>      >>>> So this register should already return 0, otherwise we have
>>      >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly.
>>      >>
>>      >> My idea here was to catch this unimplemented case in order to
>>     indicate this clearly to users. Perhaps it nudges somebody to
>>     provide a patch?
>>      >>
>>      >>>>
>>      >>>> And your model is reduced to handling create_unimp() in
>>     esdhci_realize().
>>      >>>>
>>      >>>> Am I missing something?
>>      >>>
>>      >>> The mmio ops are big endian and need to be aligned to a 4-byte
>>     boundary. It took me quite a while to debug this. So shall I just
>>     create an additional memory region for the region above
>>     SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
>>      >>
>>      >> All in all I currently don't have a better idea than keeping the
>>     custom i/o ops for the standard region and adding an additional
>>     unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to
>>     dynamically allocate memory for it where I still need to figure out
>>     how not to leak it.
>>      >
>>      > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I
>>     was able to remove the custom implementations while having big
>>     endian and the alignments proper. However, I don't see a way of
>>     adding two memory regions - with or without a container. With a
>>     container I'd have to somehow preserve the mmio attribute which is
>>     initialized by the parent class, re-initialize it with the
>>     container, and add the preserved memory region as child. This seems
>>     very fragile, esp. since the parent class has created an alias for
>>     mmio in sysbus. Without a container, one would have two memory
>>     regions that both have to be mapped separately by the caller, i.e.
>>     it burdens the caller with an implementation detail.
>>      >
>>      > Any suggestions?
>
>See https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/
>
>>     Can you share branch and how to test?
>> 
>> 
>> QEMU branch: https://github.com/shentok/qemu/tree/e500-flash <https://github.com/shentok/qemu/tree/e500-flash>
>> 
>> How to test:
>> 1. `git clone -b e500 https://github.com/shentok/buildroot.git <https://github.com/shentok/buildroot.git>`
>> 2. `cd buildroot`
>> 3. `make qemu_ppc_e500mc_defconfig`
>> 4. `make`
>> 5. `cd output/images`
>> 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 of=root.img bs=1M conv=notrunc`
>> 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive id=mydrive,if=none,file=root.img,format=raw`
>
>Could you add an Avocado-based test?

I could give it a try at least. Where would I drop the binaries?

Best regards,
Bernhard
>
>>    Welcome to Buildroot
>>    buildroot login:
>
>Regards,
>
>Phil.

Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model

[Bernhard Beschow]

Am 1. November 2022 10:49:20 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Am 31. Oktober 2022 12:11:37 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>On 30/10/22 12:46, Bernhard Beschow wrote:
>>> 
>>> 
>>> On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
>>> 
>>>     On 29/10/22 20:28, Bernhard Beschow wrote:
>>>      > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow
>>>     <shentey@gmail.com <mailto:shentey@gmail.com>>:
>>>      >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow
>>>     <shentey@gmail.com <mailto:shentey@gmail.com>>:
>>>      >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe
>>>     Mathieu-Daudé" <philmd@linaro.org <mailto:philmd@linaro.org>>:
>>>      >>>> Hi Bernhard,
>>>      >>>>
>>>      >>>> On 18/10/22 23:01, Bernhard Beschow wrote:
>>>      >>>>> Will allow e500 boards to access SD cards using just their
>>>     own devices.
>>>      >>>>>
>>>      >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com
>>>     <mailto:shentey@gmail.com>>
>>>      >>>>> ---
>>>      >>>>>    hw/sd/sdhci.c         | 120
>>>     +++++++++++++++++++++++++++++++++++++++++-
>>>      >>>>>    include/hw/sd/sdhci.h |   3 ++
>>>      >>>>>    2 files changed, 122 insertions(+), 1 deletion(-)
>>> 
>>>      >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>>>      >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>>>      >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE -
>>>     SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>>>      >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>>>      >>>> hw/arm/bcm2835_peripherals.c.
>>>      >>>>
>>>      >>>> But the ESDHC_WML register has address 0x44 and fits inside the
>>>      >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>>>      >>>> upper part of the SDHC_CAPAB register. These bits are undefined
>>>      >>>> on the spec v2, which I see you are setting in esdhci_init().
>>>      >>>> So this register should already return 0, otherwise we have
>>>      >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly.
>>>      >>
>>>      >> My idea here was to catch this unimplemented case in order to
>>>     indicate this clearly to users. Perhaps it nudges somebody to
>>>     provide a patch?
>>>      >>
>>>      >>>>
>>>      >>>> And your model is reduced to handling create_unimp() in
>>>     esdhci_realize().
>>>      >>>>
>>>      >>>> Am I missing something?
>>>      >>>
>>>      >>> The mmio ops are big endian and need to be aligned to a 4-byte
>>>     boundary. It took me quite a while to debug this. So shall I just
>>>     create an additional memory region for the region above
>>>     SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
>>>      >>
>>>      >> All in all I currently don't have a better idea than keeping the
>>>     custom i/o ops for the standard region and adding an additional
>>>     unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to
>>>     dynamically allocate memory for it where I still need to figure out
>>>     how not to leak it.
>>>      >
>>>      > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I
>>>     was able to remove the custom implementations while having big
>>>     endian and the alignments proper. However, I don't see a way of
>>>     adding two memory regions - with or without a container. With a
>>>     container I'd have to somehow preserve the mmio attribute which is
>>>     initialized by the parent class, re-initialize it with the
>>>     container, and add the preserved memory region as child. This seems
>>>     very fragile, esp. since the parent class has created an alias for
>>>     mmio in sysbus. Without a container, one would have two memory
>>>     regions that both have to be mapped separately by the caller, i.e.
>>>     it burdens the caller with an implementation detail.
>>>      >
>>>      > Any suggestions?
>>
>>See https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/
>>
>>>     Can you share branch and how to test?
>>> 
>>> 
>>> QEMU branch: https://github.com/shentok/qemu/tree/e500-flash <https://github.com/shentok/qemu/tree/e500-flash>
>>> 
>>> How to test:
>>> 1. `git clone -b e500 https://github.com/shentok/buildroot.git <https://github.com/shentok/buildroot.git>`
>>> 2. `cd buildroot`
>>> 3. `make qemu_ppc_e500mc_defconfig`
>>> 4. `make`
>>> 5. `cd output/images`
>>> 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 of=root.img bs=1M conv=notrunc`
>>> 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive id=mydrive,if=none,file=root.img,format=raw`
>>
>>Could you add an Avocado-based test?
>
>I could give it a try at least. Where would I drop the binaries?

Looks like Cederic already had a repo [1]. Let's see what I can do.

Best regards,
Bernhard

[1] https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot

>
>Best regards,
>Bernhard
>>
>>>    Welcome to Buildroot
>>>    buildroot login:
>>
>>Regards,
>>
>>Phil.
>