Re: [PATCH v5 04/14] hw/sd/sdhci: Include 'wp-inverted' property in quirk bitmask

[BALATON Zoltan]

On Mon, 10 Mar 2025, Philippe Mathieu-Daudé wrote:

Import Linux's SDHCI_QUIRK_INVERTED_WRITE_PROTECT quirk definition.

Replace 'wp_inverted' boolean by a bit in quirk bitmask.

Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
---
include/hw/sd/sdhci.h | 16 ++++++++++------
hw/arm/aspeed.c       |  2 +-
hw/sd/sdhci.c         |  6 +++---
3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 096d607f4b7..d2e4f0f0050 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -30,7 +30,14 @@
#include "hw/sd/sd.h"
#include "qom/object.h"

-/* SD/MMC host controller state */
+/*
+ * SD/MMC host controller state
+ *
+ * QEMU interface:
+ *  + QOM property "wp-inverted-quirk" inverts the Write Protect pin
+ *    polarity (by default the polarity is active low for detecting SD
+ *    card to be protected).
+ */
struct SDHCIState {
    /*< private >*/
    union {
@@ -99,11 +106,6 @@ struct SDHCIState {
    uint8_t endianness;
    uint8_t sd_spec_version;
    uint8_t uhs_mode;
-    /*
-     * Write Protect pin default active low for detecting SD card
-     * to be protected. Set wp_inverted to invert the signal.
-     */
-    bool wp_inverted;
};
typedef struct SDHCIState SDHCIState;

@@ -114,6 +116,8 @@ typedef struct SDHCIState SDHCIState;

Now that we have two adjust comment above here to say "These defines"

enum {
    /* Controller does not provide transfer-complete interrupt when not busy. */
    SDHCI_QUIRK_NO_BUSY_IRQ                     = 14,
+    /* Controller reports inverted write-protect state */
+    SDHCI_QUIRK_INVERTED_WRITE_PROTECT          = 16,
};

and I'd say keep this defines that also matches what Linux has.

#define TYPE_PCI_SDHCI "sdhci-pci"
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 98bf071139b..daee2376d50 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -412,7 +412,7 @@ static void aspeed_machine_init(MachineState *machine)
    if (amc->sdhci_wp_inverted) {
        for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
            object_property_set_bool(OBJECT(&bmc->soc->sdhci.slots[i]),
-                                     "wp-inverted", true, &error_abort);
+                                     "wp-inverted-quirk", true, &error_abort);

Why rename it? That would break command lines that use this.

Regards,
BALATON Zoltan

        }
    }
    if (machine->kernel_filename) {
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1dc942a0e06..19c600d5bfc 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -274,7 +274,7 @@ static void sdhci_set_readonly(DeviceState *dev, bool level)
{
    SDHCIState *s = (SDHCIState *)dev;

-    if (s->wp_inverted) {
+    if (s->quirks & BIT(SDHCI_QUIRK_INVERTED_WRITE_PROTECT)) {
        level = !level;
    }

@@ -1555,12 +1555,12 @@ void sdhci_common_class_init(ObjectClass *klass, const 
void *data)

static const Property sdhci_sysbus_properties[] = {
    DEFINE_SDHCI_COMMON_PROPERTIES(SDHCIState),
+    DEFINE_PROP_BIT("wp-inverted-quirk", SDHCIState, quirks,
+                    SDHCI_QUIRK_INVERTED_WRITE_PROTECT, false),
    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
                     false),
    DEFINE_PROP_LINK("dma", SDHCIState,
                     dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
-    DEFINE_PROP_BOOL("wp-inverted", SDHCIState,
-                     wp_inverted, false),
};

static void sdhci_sysbus_init(Object *obj)