[PATCH v6 0/3] ppc/e500: Add support for eSDHC

[PATCH v6 0/3] ppc/e500: Add support for eSDHC

[Philippe Mathieu-Daudé]

This is a respin of Bernhard's v4 with Freescale eSDHC implemented
as an 'UNIMP' region. See v4 cover here:
https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/

Since v5:
- Rebased (ppc-next merged)
- Properly handle big-endian

Since v4:
- Do not rename ESDHC_* definitions to USDHC_*
- Do not modify SDHCIState structure

Supersedes: <20221031115402.91912-1-philmd@linaro.org>

Philippe Mathieu-Daudé (3):
  hw/sd/sdhci: MMIO region is implemented in 32-bit accesses
  hw/sd/sdhci: Support big endian SD host controller interfaces
  hw/ppc/e500: Add Freescale eSDHC to e500plat

 docs/system/ppc/ppce500.rst | 13 ++++++++++
 hw/ppc/Kconfig              |  2 ++
 hw/ppc/e500.c               | 48 ++++++++++++++++++++++++++++++++++++-
 hw/ppc/e500.h               |  1 +
 hw/ppc/e500plat.c           |  1 +
 hw/sd/sdhci-internal.h      |  1 +
 hw/sd/sdhci.c               | 36 +++++++++++++++++++++++++---
 include/hw/sd/sdhci.h       |  1 +
 8 files changed, 99 insertions(+), 4 deletions(-)

-- 
2.38.1

[PATCH v6 1/3] hw/sd/sdhci: MMIO region is implemented in 32-bit accesses

[Philippe Mathieu-Daudé]

Tested-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 306070c872..22c758ad91 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1332,6 +1332,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
 static const MemoryRegionOps sdhci_mmio_ops = {
     .read = sdhci_read,
     .write = sdhci_write,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
     .valid = {
         .min_access_size = 1,
         .max_access_size = 4,
-- 
2.38.1

[PATCH v6 2/3] hw/sd/sdhci: Support big endian SD host controller interfaces

[Philippe Mathieu-Daudé]

Some SDHCI IP can be synthetized in various endianness:
https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc

 - CONFIG_SYS_FSL_ESDHC_BE

   ESDHC IP is in big-endian mode. Accessing ESDHC registers can be
   determined by ESDHC IP's endian mode or processor's endian mode.

Our current implementation is little-endian. In order to support
big endianness:

- Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le')
- Add an 'endianness' property to SDHCIState (default little endian)
- Set the 'io_ops' field in realize() after checking the property
- Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci-internal.h |  1 +
 hw/sd/sdhci.c          | 32 +++++++++++++++++++++++++++++---
 include/hw/sd/sdhci.h  |  1 +
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 964570f8e8..5f3765f12d 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate;
 #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
 
 #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
+    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
     DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
     DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
     DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 22c758ad91..289baa879e 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                        value >> shift, value >> shift);
 }
 
-static const MemoryRegionOps sdhci_mmio_ops = {
+static const MemoryRegionOps sdhci_mmio_le_ops = {
     .read = sdhci_read,
     .write = sdhci_write,
     .impl = {
@@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static const MemoryRegionOps sdhci_mmio_be_ops = {
+    .read = sdhci_read,
+    .write = sdhci_write,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+        .unaligned = false
+    },
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
 static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
 {
     ERRP_GUARD();
@@ -1371,8 +1386,6 @@ void sdhci_initfn(SDHCIState *s)
 
     s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
-
-    s->io_ops = &sdhci_mmio_ops;
 }
 
 void sdhci_uninitfn(SDHCIState *s)
@@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
 {
     ERRP_GUARD();
 
+    switch (s->endianness) {
+    case DEVICE_LITTLE_ENDIAN:
+        s->io_ops = &sdhci_mmio_le_ops;
+        break;
+    case DEVICE_BIG_ENDIAN:
+        s->io_ops = &sdhci_mmio_be_ops;
+        break;
+    default:
+        error_setg(errp, "Incorrect endianness");
+        return;
+    }
+
     sdhci_init_readonly_registers(s, errp);
     if (*errp) {
         return;
     }
+
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
 
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 01a64c5442..a989fca3b2 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -96,6 +96,7 @@ struct SDHCIState {
     /* Configurable properties */
     bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
     uint32_t quirks;
+    uint8_t endianness;
     uint8_t sd_spec_version;
     uint8_t uhs_mode;
     uint8_t vendor;        /* For vendor specific functionality */
-- 
2.38.1

[PATCH v6 3/3] hw/ppc/e500: Add Freescale eSDHC to e500plat

[Philippe Mathieu-Daudé]

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>
Message-Id: <20221018210146.193159-8-shentey@gmail.com>
[PMD: Simplify using create_unimplemented_device("esdhc")]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/system/ppc/ppce500.rst | 13 ++++++++++
 hw/ppc/Kconfig              |  2 ++
 hw/ppc/e500.c               | 48 ++++++++++++++++++++++++++++++++++++-
 hw/ppc/e500.h               |  1 +
 hw/ppc/e500plat.c           |  1 +
 5 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
index fa40e57d18..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
@@ -180,3 +181,15 @@ as follows:
       -kernel vmlinux \
       -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 b8d2522f45..72a311edcb 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -128,10 +128,12 @@ config E500
     select PFLASH_CFI01
     select PLATFORM_BUS
     select PPCE500_PCI
+    select SDHCI
     select SERIAL
     select MPC_I2C
     select FDT_PPC
     select DS1338
+    select UNIMP
 
 config E500PLAT
     bool
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 2fe496677c..2bef2f01cb 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -48,6 +48,8 @@
 #include "hw/net/fsl_etsec/etsec.h"
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
+#include "hw/sd/sdhci.h"
+#include "hw/misc/unimp.h"
 
 #define EPAPR_MAGIC                (0x45504150)
 #define DTC_LOAD_PAD               0x1800000
@@ -66,11 +68,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 +208,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 +574,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 +1007,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 +1018,26 @@ 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) {
+        create_unimplemented_device("esdhc",
+                                    pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
+                                    MPC85XX_ESDHC_REGS_SIZE);
+
+        /*
+         * Compatible with:
+         * - SD Host Controller Specification Version 2.0 Part A2
+         * (See MPC8569E Reference Manual)
+         */
+        dev = qdev_new(TYPE_SYSBUS_SDHCI);
+        qdev_prop_set_uint8(dev, "sd-spec-version", 2);
+        qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
+        s = SYS_BUS_DEVICE(dev);
+        sysbus_realize_and_unref(s, &error_fatal);
+        sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
+        memory_region_add_subregion(ccsr_addr_space, MPC85XX_ESDHC_REGS_OFFSET,
+                                    sysbus_mmio_get_region(s, 0));
+    }
 
     /* 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.1

Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[Bernhard Beschow]

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

On Tue, Nov 1, 2022 at 11:29 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> This is a respin of Bernhard's v4 with Freescale eSDHC implemented
> as an 'UNIMP' region. See v4 cover here:
>
> https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/
>
> Since v5:
> - Rebased (ppc-next merged)
> - Properly handle big-endian
>

Tested-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Bernhard Beschow <shentey@gmail.com>


> Since v4:
> - Do not rename ESDHC_* definitions to USDHC_*
> - Do not modify SDHCIState structure
>
> Supersedes: <20221031115402.91912-1-philmd@linaro.org>
>
> Philippe Mathieu-Daudé (3):
>   hw/sd/sdhci: MMIO region is implemented in 32-bit accesses
>   hw/sd/sdhci: Support big endian SD host controller interfaces
>   hw/ppc/e500: Add Freescale eSDHC to e500plat
>
>  docs/system/ppc/ppce500.rst | 13 ++++++++++
>  hw/ppc/Kconfig              |  2 ++
>  hw/ppc/e500.c               | 48 ++++++++++++++++++++++++++++++++++++-
>  hw/ppc/e500.h               |  1 +
>  hw/ppc/e500plat.c           |  1 +
>  hw/sd/sdhci-internal.h      |  1 +
>  hw/sd/sdhci.c               | 36 +++++++++++++++++++++++++---
>  include/hw/sd/sdhci.h       |  1 +
>  8 files changed, 99 insertions(+), 4 deletions(-)
>
> --
> 2.38.1
>
>

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

Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[Daniel Henrique Barboza]

On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
> This is a respin of Bernhard's v4 with Freescale eSDHC implemented
> as an 'UNIMP' region. See v4 cover here:
> https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/
> 
> Since v5:
> - Rebased (ppc-next merged)
> - Properly handle big-endian
> 
> Since v4:
> - Do not rename ESDHC_* definitions to USDHC_*
> - Do not modify SDHCIState structure
> 
> Supersedes: <20221031115402.91912-1-philmd@linaro.org>

Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
freeze for 7.2).


BTW, checkpatch complained about this line being too long (83 chars):


3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to e500plat)
WARNING: line over 80 characters
#150: FILE: hw/ppc/e500.c:1024:
+                                    pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,


The code except is this:

     if (pmc->has_esdhc) {
         create_unimplemented_device("esdhc",
                                     pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
                                     MPC85XX_ESDHC_REGS_SIZE);


To get rid of the warning we would need to make a python-esque identation (line
break after "(" ) or create a new variable to hold the sum. Both seems overkill
so I'll ignore the warning. Phil is welcome to re-send if he thinks it's worth
it.


And I'll follow it up with my usual plea in these cases: can we move the line size
warning to 100 chars? For QEMU 8.0? Pretty please?


Daniel


> 
> Philippe Mathieu-Daudé (3):
>    hw/sd/sdhci: MMIO region is implemented in 32-bit accesses
>    hw/sd/sdhci: Support big endian SD host controller interfaces
>    hw/ppc/e500: Add Freescale eSDHC to e500plat
> 
>   docs/system/ppc/ppce500.rst | 13 ++++++++++
>   hw/ppc/Kconfig              |  2 ++
>   hw/ppc/e500.c               | 48 ++++++++++++++++++++++++++++++++++++-
>   hw/ppc/e500.h               |  1 +
>   hw/ppc/e500plat.c           |  1 +
>   hw/sd/sdhci-internal.h      |  1 +
>   hw/sd/sdhci.c               | 36 +++++++++++++++++++++++++---
>   include/hw/sd/sdhci.h       |  1 +
>   8 files changed, 99 insertions(+), 4 deletions(-)
> 

Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[BALATON Zoltan]

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

On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:
> On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
>> This is a respin of Bernhard's v4 with Freescale eSDHC implemented
>> as an 'UNIMP' region. See v4 cover here:
>> https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/
>> 
>> Since v5:
>> - Rebased (ppc-next merged)
>> - Properly handle big-endian
>> 
>> Since v4:
>> - Do not rename ESDHC_* definitions to USDHC_*
>> - Do not modify SDHCIState structure
>> 
>> Supersedes: <20221031115402.91912-1-philmd@linaro.org>
>
> Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
> freeze for 7.2).

Could you please always use ppc-next to queue patches for the next 
upcoming version and ppc-7.2 for the current version? Unless this makes 
your workflow harder in which case ignore this but the reason I ask is 
because then it's enough for me to only track ppc-next if I need to rebase 
patches on that and don't have to add a new branch at every release 
(unless I have some patches to rebase on it during a freeze but that's 
less likely than rebasing on your queued patches for the next release xo 
using version for the current branch and keep next for the future versions 
makes more sense to me).

> BTW, checkpatch complained about this line being too long (83 chars):
>
>
> 3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to 
> e500plat)
> WARNING: line over 80 characters
> #150: FILE: hw/ppc/e500.c:1024:
> +                                    pmc->ccsrbar_base + 
> MPC85XX_ESDHC_REGS_OFFSET,
>
>
> The code except is this:
>
>    if (pmc->has_esdhc) {
>        create_unimplemented_device("esdhc",
>                                    pmc->ccsrbar_base + 
> MPC85XX_ESDHC_REGS_OFFSET,
>                                    MPC85XX_ESDHC_REGS_SIZE);
>
>
> To get rid of the warning we would need to make a python-esque identation 
> (line
> break after "(" ) or create a new variable to hold the sum. Both seems 
> overkill
> so I'll ignore the warning. Phil is welcome to re-send if he thinks it's 
> worth
> it.

Or you could break indentation and not start at the ( but 3 chars back. I.e.:

create_unimplemented_device("esdhc",
                          pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
                          MPC85XX_ESDHC_REGS_SIZE);

But I think it can be just ignored in this case.

> And I'll follow it up with my usual plea in these cases: can we move the line 
> size warning to 100 chars? For QEMU 8.0? Pretty please?

I think the consensus was to keep 80 columns if possible, this is good 
becuase you can open more files side by side (although it does not match 
well with the long _ naming convention of glib and qemu)  but we have a 
distinction between checkpatch warning and error in line length. I think 
it will give error at 90 chars but as long as it's just warns that means: 
fix it if you can but in rare cases if it's more readable with a slightly 
longer line then it is still acceptable. I think that's the case here, 
splitting the line would be less readable than a few chars longer line.

Regards,
BALATON Zoltan

Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[Daniel Henrique Barboza]

On 11/3/22 09:51, BALATON Zoltan wrote:
> On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:
>> On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
>>> This is a respin of Bernhard's v4 with Freescale eSDHC implemented
>>> as an 'UNIMP' region. See v4 cover here:
>>> https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/
>>>
>>> Since v5:
>>> - Rebased (ppc-next merged)
>>> - Properly handle big-endian
>>>
>>> Since v4:
>>> - Do not rename ESDHC_* definitions to USDHC_*
>>> - Do not modify SDHCIState structure
>>>
>>> Supersedes: <20221031115402.91912-1-philmd@linaro.org>
>>
>> Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
>> freeze for 7.2).
> 
> Could you please always use ppc-next to queue patches for the next upcoming version and ppc-7.2 for the current version? Unless this makes your workflow harder in which case ignore this but the reason I ask is because then it's enough for me to only track ppc-next if I need to rebase patches on that and don't have to add a new branch at every release (unless I have some patches to rebase on it during a freeze but that's less likely than rebasing on your queued patches for the next release xo using version for the current branch and keep next for the future versions makes more sense to me).

Note that doing "ppc-7.2" for the current release and ppc-next for the
next release will not prevent you from adding a new branch at every
release, e.g. for the next release you would need to add a ppc-8.0
branch.

'ppc-next' is used like a standard, a way of telling 'this is the next
pull request that is going upstream'. Can we change it? Sure, but if the
idea is to avoid new branches every new release then I suggest the
following:

- ppc-next: keep it as is
- ppc-next-release/ppc-future: branch that will host any code for the next
release during the code freeze window. Note that this branch will become
'ppc-next' when the new release cycle begins


This would avoid changing everyone's workflow with the current ppc-next
usage, while also standardize a branch for the future release patches
during freeze.



> 
>> BTW, checkpatch complained about this line being too long (83 chars):
>>
>>
>> 3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to e500plat)
>> WARNING: line over 80 characters
>> #150: FILE: hw/ppc/e500.c:1024:
>> +                                    pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
>>
>>
>> The code except is this:
>>
>>    if (pmc->has_esdhc) {
>>        create_unimplemented_device("esdhc",
>>                                    pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
>>                                    MPC85XX_ESDHC_REGS_SIZE);
>>
>>
>> To get rid of the warning we would need to make a python-esque identation (line
>> break after "(" ) or create a new variable to hold the sum. Both seems overkill
>> so I'll ignore the warning. Phil is welcome to re-send if he thinks it's worth
>> it.
> 
> Or you could break indentation and not start at the ( but 3 chars back. I.e.:
> 
> create_unimplemented_device("esdhc",
>                           pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
>                           MPC85XX_ESDHC_REGS_SIZE);
> 
> But I think it can be just ignored in this case.
> 
>> And I'll follow it up with my usual plea in these cases: can we move the line size warning to 100 chars? For QEMU 8.0? Pretty please?
> 
> I think the consensus was to keep 80 columns if possible, this is good becuase you can open more files side by side (although it does not match well with the long _ naming convention of glib and qemu)  but we have a distinction between checkpatch warning and error in line length. I think it will give error at 90 chars but as long as it's just warns that means: fix it if you can but in rare cases if it's more readable with a slightly longer line then it is still acceptable. I think that's the case here, splitting the line would be less readable than a few chars longer line.

Yeah I know that we can usually ignore these warnings. I keep bringing
this up because it's weird to keep bothering with 80 chars per line when
people are using 28" flat screen monitors, multiple screen desktops
and so on.


Thanks,


Daniel

> 
> Regards,
> BALATON Zoltan

Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[Philippe Mathieu-Daudé]

On 3/11/22 16:13, Daniel Henrique Barboza wrote:
> On 11/3/22 09:51, BALATON Zoltan wrote:
>> On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:

>>> Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
>>> freeze for 7.2).
>>
>> Could you please always use ppc-next to queue patches for the next 
>> upcoming version and ppc-7.2 for the current version? Unless this 
>> makes your workflow harder in which case ignore this but the reason I 
>> ask is because then it's enough for me to only track ppc-next if I 
>> need to rebase patches on that and don't have to add a new branch at 
>> every release (unless I have some patches to rebase on it during a 
>> freeze but that's less likely than rebasing on your queued patches for 
>> the next release xo using version for the current branch and keep next 
>> for the future versions makes more sense to me).
> 
> Note that doing "ppc-7.2" for the current release and ppc-next for the
> next release will not prevent you from adding a new branch at every
> release, e.g. for the next release you would need to add a ppc-8.0
> branch.
> 
> 'ppc-next' is used like a standard, a way of telling 'this is the next
> pull request that is going upstream'. Can we change it? Sure, but if the
> idea is to avoid new branches every new release then I suggest the
> following:
> 
> - ppc-next: keep it as is

FWIW I use mips-next the same way,

> - ppc-next-release/ppc-future: branch that will host any code for the next
> release during the code freeze window. Note that this branch will become
> 'ppc-next' when the new release cycle begins

and use mips-fixes during freeze. My 2 cents, not sure it helps Zoltan.

> This would avoid changing everyone's workflow with the current ppc-next
> usage, while also standardize a branch for the future release patches
> during freeze.

Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[BALATON Zoltan]

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

On Thu, 3 Nov 2022, Daniel Henrique Barboza wrote:
> On 11/3/22 09:51, BALATON Zoltan wrote:
>> On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:
>>> On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
>>>> This is a respin of Bernhard's v4 with Freescale eSDHC implemented
>>>> as an 'UNIMP' region. See v4 cover here:
>>>> https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/
>>>> 
>>>> Since v5:
>>>> - Rebased (ppc-next merged)
>>>> - Properly handle big-endian
>>>> 
>>>> Since v4:
>>>> - Do not rename ESDHC_* definitions to USDHC_*
>>>> - Do not modify SDHCIState structure
>>>> 
>>>> Supersedes: <20221031115402.91912-1-philmd@linaro.org>
>>> 
>>> Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
>>> freeze for 7.2).
>> 
>> Could you please always use ppc-next to queue patches for the next upcoming 
>> version and ppc-7.2 for the current version? Unless this makes your 
>> workflow harder in which case ignore this but the reason I ask is because 
>> then it's enough for me to only track ppc-next if I need to rebase patches 
>> on that and don't have to add a new branch at every release (unless I have 
>> some patches to rebase on it during a freeze but that's less likely than 
>> rebasing on your queued patches for the next release xo using version for 
>> the current branch and keep next for the future versions makes more sense 
>> to me).
>
> Note that doing "ppc-7.2" for the current release and ppc-next for the
> next release will not prevent you from adding a new branch at every
> release, e.g. for the next release you would need to add a ppc-8.0
> branch.
>
> 'ppc-next' is used like a standard, a way of telling 'this is the next
> pull request that is going upstream'. Can we change it? Sure, but if the
> idea is to avoid new branches every new release then I suggest the
> following:
>
> - ppc-next: keep it as is
> - ppc-next-release/ppc-future: branch that will host any code for the next
> release during the code freeze window. Note that this branch will become
> 'ppc-next' when the new release cycle begins
>
>
> This would avoid changing everyone's workflow with the current ppc-next
> usage, while also standardize a branch for the future release patches
> during freeze.

As I said above if this means changing your or other's workflow making it 
more inconvenient for you then just ignore my request as it does not worth 
the trouble this might cause for others. So only change it if it's not 
much trouble.

As for using next for future release and versioned branch for current one 
in freeze this might not completely eliminate the need to track it for me 
but makes it much less likely as I only need the freeze branch when I have 
to submit a fix during the freeze AND you also already have other fixes 
queued AND those fixes conflict with my patch. This is very unlikely so in 
most cases I can just base the fix on master during the freeze and not 
care about the freeze branch only in very rare cases. It's much more 
likely that I have outstanding patches that I have to rebase for the next 
release when you already queued patches e.g. during a freeze (or during 
development before pull requests but the latter case already uses 
ppc-next).

Philippe's solution to use something like ppc-freze, -fixes whatever 
without a version number for pathces during a freeze would also work as 
then I only need to track those two branches but this would also break 
your condition of always using ppc-next for the next pull request so again 
if this causes any trouble for others then just leave it as it is.

>>> BTW, checkpatch complained about this line being too long (83 chars):
>>> 
>>> 
>>> 3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to 
>>> e500plat)
>>> WARNING: line over 80 characters
>>> #150: FILE: hw/ppc/e500.c:1024:
>>> +                                    pmc->ccsrbar_base + 
>>> MPC85XX_ESDHC_REGS_OFFSET,
>>> 
>>> 
>>> The code except is this:
>>> 
>>>    if (pmc->has_esdhc) {
>>>        create_unimplemented_device("esdhc",
>>>                                    pmc->ccsrbar_base + 
>>> MPC85XX_ESDHC_REGS_OFFSET,
>>>                                    MPC85XX_ESDHC_REGS_SIZE);
>>> 
>>> 
>>> To get rid of the warning we would need to make a python-esque identation 
>>> (line
>>> break after "(" ) or create a new variable to hold the sum. Both seems 
>>> overkill
>>> so I'll ignore the warning. Phil is welcome to re-send if he thinks it's 
>>> worth
>>> it.
>> 
>> Or you could break indentation and not start at the ( but 3 chars back. 
>> I.e.:
>> 
>> create_unimplemented_device("esdhc",
>>                           pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
>>                           MPC85XX_ESDHC_REGS_SIZE);
>> 
>> But I think it can be just ignored in this case.
>> 
>>> And I'll follow it up with my usual plea in these cases: can we move the 
>>> line size warning to 100 chars? For QEMU 8.0? Pretty please?
>> 
>> I think the consensus was to keep 80 columns if possible, this is good 
>> becuase you can open more files side by side (although it does not match 
>> well with the long _ naming convention of glib and qemu)  but we have a 
>> distinction between checkpatch warning and error in line length. I think it 
>> will give error at 90 chars but as long as it's just warns that means: fix 
>> it if you can but in rare cases if it's more readable with a slightly 
>> longer line then it is still acceptable. I think that's the case here, 
>> splitting the line would be less readable than a few chars longer line.
>
> Yeah I know that we can usually ignore these warnings. I keep bringing
> this up because it's weird to keep bothering with 80 chars per line when
> people are using 28" flat screen monitors, multiple screen desktops
> and so on.

Not everyone does. I mostly use a single screen which is not 28" and still 
want to open more than one window without switching desktops or some may 
use laptops with smaller screens etc. 80 chars may be an old convention 
that could be raised now but probably this would just result in some files 
being formatted for longer lines while most of the older code still having 
80 chars so it just brings inconsistency. Reformatting everything would 
not work either so maybe it's easier to just stick with it now.

Regards,
BALATON Zoltan

Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC

[Daniel Henrique Barboza]

On 11/3/22 23:31, BALATON Zoltan wrote:
> On Thu, 3 Nov 2022, Daniel Henrique Barboza wrote:
>> On 11/3/22 09:51, BALATON Zoltan wrote:
>>> On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:
>>>> On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
>>>>> This is a respin of Bernhard's v4 with Freescale eSDHC implemented
>>>>> as an 'UNIMP' region. See v4 cover here:
>>>>> https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shentey@gmail.com/
>>>>>
>>>>> Since v5:
>>>>> - Rebased (ppc-next merged)
>>>>> - Properly handle big-endian
>>>>>
>>>>> Since v4:
>>>>> - Do not rename ESDHC_* definitions to USDHC_*
>>>>> - Do not modify SDHCIState structure
>>>>>
>>>>> Supersedes: <20221031115402.91912-1-philmd@linaro.org>
>>>>
>>>> Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
>>>> freeze for 7.2).
>>>
>>> Could you please always use ppc-next to queue patches for the next upcoming version and ppc-7.2 for the current version? Unless this makes your workflow harder in which case ignore this but the reason I ask is because then it's enough for me to only track ppc-next if I need to rebase patches on that and don't have to add a new branch at every release (unless I have some patches to rebase on it during a freeze but that's less likely than rebasing on your queued patches for the next release xo using version for the current branch and keep next for the future versions makes more sense to me).
>>
>> Note that doing "ppc-7.2" for the current release and ppc-next for the
>> next release will not prevent you from adding a new branch at every
>> release, e.g. for the next release you would need to add a ppc-8.0
>> branch.
>>
>> 'ppc-next' is used like a standard, a way of telling 'this is the next
>> pull request that is going upstream'. Can we change it? Sure, but if the
>> idea is to avoid new branches every new release then I suggest the
>> following:
>>
>> - ppc-next: keep it as is
>> - ppc-next-release/ppc-future: branch that will host any code for the next
>> release during the code freeze window. Note that this branch will become
>> 'ppc-next' when the new release cycle begins
>>
>>
>> This would avoid changing everyone's workflow with the current ppc-next
>> usage, while also standardize a branch for the future release patches
>> during freeze.
> 
> As I said above if this means changing your or other's workflow making it more inconvenient for you then just ignore my request as it does not worth the trouble this might cause for others. So only change it if it's not much trouble.
> 
> As for using next for future release and versioned branch for current one in freeze this might not completely eliminate the need to track it for me but makes it much less likely as I only need the freeze branch when I have to submit a fix during the freeze AND you also already have other fixes queued AND those fixes conflict with my patch. This is very unlikely so in most cases I can just base the fix on master during the freeze and not care about the freeze branch only in very rare cases. It's much more likely that I have outstanding patches that I have to rebase for the next release when you already queued patches e.g. during a freeze (or during development before pull requests but the latter case already uses ppc-next).
> 
> Philippe's solution to use something like ppc-freze, -fixes whatever without a version number for pathces during a freeze would also work as then I only need to track those two branches but this would also break your condition of always using ppc-next for the next pull request so again if this causes any trouble for others then just leave it as it is.


I think I'll actually make the change I proposed:

- ppc-next will always be the next incoming content for the current release,
like it's has been for some time now.

- ppc-future will consist of ppc-next + patches that didn't make the freeze.
And yeah, every change I make on ppc-next I'll rebase ppc-future accordingly.


I won't be using any versioned branch like ppc-7.2, ppc-8.0 or something.
These two branches can be used regardless of the current release number.

I'll do that later today.


Daniel

> 
>>>> BTW, checkpatch complained about this line being too long (83 chars):
>>>>
>>>>
>>>> 3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to e500plat)
>>>> WARNING: line over 80 characters
>>>> #150: FILE: hw/ppc/e500.c:1024:
>>>> +                                    pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
>>>>
>>>>
>>>> The code except is this:
>>>>
>>>>    if (pmc->has_esdhc) {
>>>>        create_unimplemented_device("esdhc",
>>>>                                    pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
>>>>                                    MPC85XX_ESDHC_REGS_SIZE);
>>>>
>>>>
>>>> To get rid of the warning we would need to make a python-esque identation (line
>>>> break after "(" ) or create a new variable to hold the sum. Both seems overkill
>>>> so I'll ignore the warning. Phil is welcome to re-send if he thinks it's worth
>>>> it.
>>>
>>> Or you could break indentation and not start at the ( but 3 chars back. I.e.:
>>>
>>> create_unimplemented_device("esdhc",
>>>                           pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
>>>                           MPC85XX_ESDHC_REGS_SIZE);
>>>
>>> But I think it can be just ignored in this case.
>>>
>>>> And I'll follow it up with my usual plea in these cases: can we move the line size warning to 100 chars? For QEMU 8.0? Pretty please?
>>>
>>> I think the consensus was to keep 80 columns if possible, this is good becuase you can open more files side by side (although it does not match well with the long _ naming convention of glib and qemu)  but we have a distinction between checkpatch warning and error in line length. I think it will give error at 90 chars but as long as it's just warns that means: fix it if you can but in rare cases if it's more readable with a slightly longer line then it is still acceptable. I think that's the case here, splitting the line would be less readable than a few chars longer line.
>>
>> Yeah I know that we can usually ignore these warnings. I keep bringing
>> this up because it's weird to keep bothering with 80 chars per line when
>> people are using 28" flat screen monitors, multiple screen desktops
>> and so on.
> 
> Not everyone does. I mostly use a single screen which is not 28" and still want to open more than one window without switching desktops or some may use laptops with smaller screens etc. 80 chars may be an old convention that could be raised now but probably this would just result in some files being formatted for longer lines while most of the older code still having 80 chars so it just brings inconsistency. Reformatting everything would not work either so maybe it's easier to just stick with it now.
> 
> Regards,
> BALATON Zoltan

Re: [PATCH v6 2/3] hw/sd/sdhci: Support big endian SD host controller interfaces

[Guenter Roeck]

Hi,

On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote:
> Some SDHCI IP can be synthetized in various endianness:
> https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc
> 
>  - CONFIG_SYS_FSL_ESDHC_BE
> 
>    ESDHC IP is in big-endian mode. Accessing ESDHC registers can be
>    determined by ESDHC IP's endian mode or processor's endian mode.
> 
> Our current implementation is little-endian. In order to support
> big endianness:
> 
> - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le')
> - Add an 'endianness' property to SDHCIState (default little endian)
> - Set the 'io_ops' field in realize() after checking the property
> - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

With this patch in place (in qemu v8.0), I can no longer boot linux
from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations.
I get the following persistent errors.

[   12.210101] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
[   12.213222] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
[   12.215072] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
[   12.218766] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
[   12.220441] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
[   12.221542] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
[   12.241544] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
[   12.242608] sdhci-esdhc-imx 2190000.mmc: card clock still not stable in 100us!.

The emulations start to work again after reverting this patch.

Guenter

> ---
>  hw/sd/sdhci-internal.h |  1 +
>  hw/sd/sdhci.c          | 32 +++++++++++++++++++++++++++++---
>  include/hw/sd/sdhci.h  |  1 +
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 964570f8e8..5f3765f12d 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate;
>  #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>  
>  #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> +    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
>      DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>      DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>      DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 22c758ad91..289baa879e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>                         value >> shift, value >> shift);
>  }
>  
> -static const MemoryRegionOps sdhci_mmio_ops = {
> +static const MemoryRegionOps sdhci_mmio_le_ops = {
>      .read = sdhci_read,
>      .write = sdhci_write,
>      .impl = {
> @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static const MemoryRegionOps sdhci_mmio_be_ops = {
> +    .read = sdhci_read,
> +    .write = sdhci_write,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +        .unaligned = false
> +    },
> +    .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
>  static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -1371,8 +1386,6 @@ void sdhci_initfn(SDHCIState *s)
>  
>      s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
>      s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
> -
> -    s->io_ops = &sdhci_mmio_ops;
>  }
>  
>  void sdhci_uninitfn(SDHCIState *s)
> @@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>  {
>      ERRP_GUARD();
>  
> +    switch (s->endianness) {
> +    case DEVICE_LITTLE_ENDIAN:
> +        s->io_ops = &sdhci_mmio_le_ops;
> +        break;
> +    case DEVICE_BIG_ENDIAN:
> +        s->io_ops = &sdhci_mmio_be_ops;
> +        break;
> +    default:
> +        error_setg(errp, "Incorrect endianness");
> +        return;
> +    }
> +
>      sdhci_init_readonly_registers(s, errp);
>      if (*errp) {
>          return;
>      }
> +
>      s->buf_maxsz = sdhci_get_fifolen(s);
>      s->fifo_buffer = g_malloc0(s->buf_maxsz);
>  
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 01a64c5442..a989fca3b2 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -96,6 +96,7 @@ struct SDHCIState {
>      /* Configurable properties */
>      bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
>      uint32_t quirks;
> +    uint8_t endianness;
>      uint8_t sd_spec_version;
>      uint8_t uhs_mode;
>      uint8_t vendor;        /* For vendor specific functionality */

Re: [PATCH v6 2/3] hw/sd/sdhci: Support big endian SD host controller interfaces

[Guenter Roeck]

On Sat, Apr 29, 2023 at 01:46:26PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote:
> > Some SDHCI IP can be synthetized in various endianness:
> > https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc
> > 
> >  - CONFIG_SYS_FSL_ESDHC_BE
> > 
> >    ESDHC IP is in big-endian mode. Accessing ESDHC registers can be
> >    determined by ESDHC IP's endian mode or processor's endian mode.
> > 
> > Our current implementation is little-endian. In order to support
> > big endianness:
> > 
> > - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le')
> > - Add an 'endianness' property to SDHCIState (default little endian)
> > - Set the 'io_ops' field in realize() after checking the property
> > - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> With this patch in place (in qemu v8.0), I can no longer boot linux
> from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations.
> I get the following persistent errors.
> 
> [   12.210101] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [   12.213222] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [   12.215072] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [   12.218766] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [   12.220441] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [   12.221542] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [   12.241544] sdhci-esdhc-imx 2190000.mmc: esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [   12.242608] sdhci-esdhc-imx 2190000.mmc: card clock still not stable in 100us!.
> 
> The emulations start to work again after reverting this patch.
> 
Cause explained below.

> Guenter
> 
> > ---
> >  hw/sd/sdhci-internal.h |  1 +
> >  hw/sd/sdhci.c          | 32 +++++++++++++++++++++++++++++---
> >  include/hw/sd/sdhci.h  |  1 +
> >  3 files changed, 31 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> > index 964570f8e8..5f3765f12d 100644
> > --- a/hw/sd/sdhci-internal.h
> > +++ b/hw/sd/sdhci-internal.h
> > @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate;
> >  #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
> >  
> >  #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> > +    DEFINE_PROP_UINT8("endianness", _state, endianness, DEVICE_LITTLE_ENDIAN), \
> >      DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
> >      DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
> >      DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 22c758ad91..289baa879e 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> >                         value >> shift, value >> shift);
> >  }
> >  
> > -static const MemoryRegionOps sdhci_mmio_ops = {
> > +static const MemoryRegionOps sdhci_mmio_le_ops = {
> >      .read = sdhci_read,
> >      .write = sdhci_write,
> >      .impl = {
> > @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = {
> >      .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >  
> > +static const MemoryRegionOps sdhci_mmio_be_ops = {
> > +    .read = sdhci_read,
> > +    .write = sdhci_write,
> > +    .impl = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> > +    },
> > +    .valid = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 4,
> > +        .unaligned = false
> > +    },
> > +    .endianness = DEVICE_BIG_ENDIAN,
> > +};
> > +
> >  static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
> >  {
> >      ERRP_GUARD();
> > @@ -1371,8 +1386,6 @@ void sdhci_initfn(SDHCIState *s)
> >  
> >      s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
> >      s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
> > -
> > -    s->io_ops = &sdhci_mmio_ops;

The reason for initializing io_ops here is that the same driver also
supports imx_usdhc. That function also sets io_ops to usdhc specific
io ops. This is now overwritten by ...

> >  }
> >  
> >  void sdhci_uninitfn(SDHCIState *s)
> > @@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
> >  {
> >      ERRP_GUARD();
> >  
> > +    switch (s->endianness) {
> > +    case DEVICE_LITTLE_ENDIAN:
> > +        s->io_ops = &sdhci_mmio_le_ops;
> > +        break;
> > +    case DEVICE_BIG_ENDIAN:
> > +        s->io_ops = &sdhci_mmio_be_ops;
> > +        break;
> > +    default:
> > +        error_setg(errp, "Incorrect endianness");
> > +        return;
> > +    }
> > +

... this code which runs later and never sets usdhc_mmio_ops.
Consequently io_ops now points to the wrong ops functions for imx.

Guenter

Re: [PATCH v6 3/3] hw/ppc/e500: Add Freescale eSDHC to e500plat

[Thomas Huth]

On 01/11/2022 23.29, Philippe Mathieu-Daudé 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>
> Message-Id: <20221018210146.193159-8-shentey@gmail.com>
> [PMD: Simplify using create_unimplemented_device("esdhc")]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

  Hi!

I recently noticed that the QEMU advent calendar 2018 day 19 image 
(https://www.qemu-advent-calendar.org/2018/download/day19.tar.xz) is 
flooding the console with "mmc0: Internal clock never stabilised" messages 
when it's started like this:

  /qemu-system-ppc64 -vga none -nographic -monitor none -M ppce500 -cpu 
e5500  -kernel ../day19/uImage

This was not the case when I assembled the image in 2018. I bisected the 
problem and apparently this patch here is causing the problem. Do you know 
whether there is a way to fix this again?

  Thanks,
   Thomas

Re: [PATCH v6 3/3] hw/ppc/e500: Add Freescale eSDHC to e500plat

[BALATON Zoltan]

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

On Thu, 17 Apr 2025, Thomas Huth wrote:
> On 01/11/2022 23.29, Philippe Mathieu-Daudé 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>
>> Message-Id: <20221018210146.193159-8-shentey@gmail.com>
>> [PMD: Simplify using create_unimplemented_device("esdhc")]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>
> Hi!
>
> I recently noticed that the QEMU advent calendar 2018 day 19 image 
> (https://www.qemu-advent-calendar.org/2018/download/day19.tar.xz) is flooding 
> the console with "mmc0: Internal clock never stabilised" messages when it's 
> started like this:
>
> /qemu-system-ppc64 -vga none -nographic -monitor none -M ppce500 -cpu e5500 
> -kernel ../day19/uImage
>
> This was not the case when I assembled the image in 2018. I bisected the 
> problem and apparently this patch here is causing the problem. Do you know 
> whether there is a way to fix this again?

That patch added the MMC/SD controller so no wonder it did not appear 
before. The error seems to be in Linux esdhci driver where it checks for:

sdhci_readl(host, ESDHC_PRSSTAT) & ESDHC_CLOCK_STABLE

So I think that bit should be set somewhere. Berhard had some patches for 
e500 so maybe there's already a fix for this somewhere.

Regards,
BALATON Zoltan

Re: [PATCH v6 3/3] hw/ppc/e500: Add Freescale eSDHC to e500plat

[Bernhard Beschow]

Am 17. April 2025 21:40:44 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Thu, 17 Apr 2025, Thomas Huth wrote:
>> On 01/11/2022 23.29, Philippe Mathieu-Daudé 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>
>>> Message-Id: <20221018210146.193159-8-shentey@gmail.com>
>>> [PMD: Simplify using create_unimplemented_device("esdhc")]
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>> 
>> Hi!
>> 
>> I recently noticed that the QEMU advent calendar 2018 day 19 image (https://www.qemu-advent-calendar.org/2018/download/day19.tar.xz) is flooding the console with "mmc0: Internal clock never stabilised" messages when it's started like this:
>> 
>> /qemu-system-ppc64 -vga none -nographic -monitor none -M ppce500 -cpu e5500 -kernel ../day19/uImage
>> 
>> This was not the case when I assembled the image in 2018. I bisected the problem and apparently this patch here is causing the problem. Do you know whether there is a way to fix this again?
>
>That patch added the MMC/SD controller so no wonder it did not appear before. The error seems to be in Linux esdhci driver where it checks for:
>
>sdhci_readl(host, ESDHC_PRSSTAT) & ESDHC_CLOCK_STABLE
>
>So I think that bit should be set somewhere. Berhard had some patches for e500 so maybe there's already a fix for this somewhere.

This logic is already implemented in the USDHC device model. Given that the e500 and the i.mx machines need the same modifications to make u-boot work, and given that the imx25 machine also uses the USDHC model even though the original has an ESDHC, the USDHC might be a better fit here. AFAICS only the interrupt bits specific to the ESDHC need special treatment here -- which could be achieved e.g. by subclassing.

I could send a patch for that. Volunteers ahead for a timely solution.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan