[PATCH v45 0/3] hw/sd/sdcard: Cleanups before adding eMMC support

[PATCH v45 0/3] hw/sd/sdcard: Cleanups before adding eMMC support

[Philippe Mathieu-Daudé]

(patches from v42 already reviewed not reposted)

- Addressed review comments from Daniel & Luc wrt migration
- Remove old comment

Philippe Mathieu-Daudé (3):
  hw/sd/sdcard: Remove leftover comment about removed 'spi' Property
  hw/sd/sdcard: Use spec v3.01 by default
  hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

 hw/core/machine.c |  1 +
 hw/sd/sd.c        | 35 ++++++++++++++++++++---------------
 2 files changed, 21 insertions(+), 15 deletions(-)

-- 
2.41.0

[PATCH v45 1/3] hw/sd/sdcard: Remove leftover comment about removed 'spi' Property

[Philippe Mathieu-Daudé]

Commit c3287c0f70 ("hw/sd: Introduce a "sd-card" SPI variant
model") removed the 'spi' property. Remove the comment left
over.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b158402704..53767beaf8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2473,10 +2473,6 @@ static Property sd_properties[] = {
     DEFINE_PROP_UINT8("spec_version", SDState,
                       spec_version, SD_PHY_SPECv2_00_VERS),
     DEFINE_PROP_DRIVE("drive", SDState, blk),
-    /* We do not model the chip select pin, so allow the board to select
-     * whether card should be in SSI or MMC/SD mode.  It is also up to the
-     * board to ensure that ssi transfers only occur when the chip select
-     * is asserted.  */
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.41.0

Re: [PATCH v45 1/3] hw/sd/sdcard: Remove leftover comment about removed 'spi' Property

[Manos Pitsidianakis]

On Wed, 03 Jul 2024 11:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Commit c3287c0f70 ("hw/sd: Introduce a "sd-card" SPI variant
>model") removed the 'spi' property. Remove the comment left
>over.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/sd/sd.c | 4 ----
> 1 file changed, 4 deletions(-)
>
>diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>index b158402704..53767beaf8 100644
>--- a/hw/sd/sd.c
>+++ b/hw/sd/sd.c
>@@ -2473,10 +2473,6 @@ static Property sd_properties[] = {
>     DEFINE_PROP_UINT8("spec_version", SDState,
>                       spec_version, SD_PHY_SPECv2_00_VERS),
>     DEFINE_PROP_DRIVE("drive", SDState, blk),
>-    /* We do not model the chip select pin, so allow the board to select
>-     * whether card should be in SSI or MMC/SD mode.  It is also up to the
>-     * board to ensure that ssi transfers only occur when the chip select
>-     * is asserted.  */
>     DEFINE_PROP_END_OF_LIST()
> };
> 
>-- 
>2.41.0
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

[PATCH v45 2/3] hw/sd/sdcard: Use spec v3.01 by default

[Philippe Mathieu-Daudé]

Recent SDHCI expect cards to support the v3.01 spec
to negociate lower I/O voltage. Select it by default.

Versioned machine types with a version of 9.0 or
earlier retain the old default (spec v2.00).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
v43: update versioned machines (danpb)
---
 hw/core/machine.c | 1 +
 hw/sd/sd.c        | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 655d75c21f..4377f943d5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -38,6 +38,7 @@ GlobalProperty hw_compat_9_0[] = {
     {"arm-cpu", "backcompat-cntfrq", "true" },
     {"scsi-disk-base", "migrate-emulated-scsi-request", "false" },
     {"vfio-pci", "skip-vsc-check", "false" },
+    {"sd-card", "spec_version", "2" },
 };
 const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
 
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 53767beaf8..a08a452d81 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2471,7 +2471,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
 
 static Property sd_properties[] = {
     DEFINE_PROP_UINT8("spec_version", SDState,
-                      spec_version, SD_PHY_SPECv2_00_VERS),
+                      spec_version, SD_PHY_SPECv3_01_VERS),
     DEFINE_PROP_DRIVE("drive", SDState, blk),
     DEFINE_PROP_END_OF_LIST()
 };
-- 
2.41.0

[PATCH v45 3/3] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

[Philippe Mathieu-Daudé]

"General command" (GEN_CMD, CMD56) is described as:

  GEN_CMD is the same as the single block read or write
  commands (CMD24 or CMD17). The difference is that [...]
  the data block is not a memory payload data but has a
  vendor specific format and meaning.

Thus this block must not be stored overwriting data block
on underlying storage drive. Keep it in a dedicated
'vendor_data[]' array.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v43: Do not re-use VMSTATE_UNUSED_V (danpb)
v44: Use subsection (Luc)
v45: Remove APP_READ_BLOCK/APP_WRITE_BLOCK macros
---
 hw/sd/sd.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a08a452d81..5d58937dd4 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -153,6 +153,8 @@ struct SDState {
     uint32_t data_offset;
     size_t data_size;
     uint8_t data[512];
+    uint8_t vendor_data[512];
+
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
@@ -719,6 +721,7 @@ static void sd_reset(DeviceState *dev)
     sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
     sd->wp_group_bits = sect;
     sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
+    memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data));
     memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = INVALID_ADDRESS;
     sd->erase_end = INVALID_ADDRESS;
@@ -793,6 +796,16 @@ static const VMStateDescription sd_ocr_vmstate = {
     },
 };
 
+static const VMStateDescription sd_vendordata_vmstate = {
+    .name = "sd-card/vendor_data-state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static int sd_vmstate_pre_load(void *opaque)
 {
     SDState *sd = opaque;
@@ -840,6 +853,7 @@ static const VMStateDescription sd_vmstate = {
     },
     .subsections = (const VMStateDescription * const []) {
         &sd_ocr_vmstate,
+        &sd_vendordata_vmstate,
         NULL
     },
 };
@@ -902,9 +916,6 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
     }
 }
 
-#define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
-#define APP_WRITE_BLOCK(a, len)
-
 static void sd_erase(SDState *sd)
 {
     uint64_t erase_start = sd->erase_start;
@@ -2187,9 +2198,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
         break;
 
     case 56:  /* CMD56:  GEN_CMD */
-        sd->data[sd->data_offset ++] = value;
-        if (sd->data_offset >= sd->blk_len) {
-            APP_WRITE_BLOCK(sd->data_start, sd->data_offset);
+        sd->vendor_data[sd->data_offset++] = value;
+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
             sd->state = sd_transfer_state;
         }
         break;
@@ -2261,12 +2271,11 @@ uint8_t sd_read_byte(SDState *sd)
         break;
 
     case 56:  /* CMD56:  GEN_CMD */
-        if (sd->data_offset == 0)
-            APP_READ_BLOCK(sd->data_start, sd->blk_len);
-        ret = sd->data[sd->data_offset ++];
+        ret = sd->vendor_data[sd->data_offset++];
 
-        if (sd->data_offset >= sd->blk_len)
+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
             sd->state = sd_transfer_state;
+        }
         break;
 
     default:
-- 
2.41.0

Re: [PATCH v45 3/3] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

[Manos Pitsidianakis]

On Wed, 03 Jul 2024 11:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>"General command" (GEN_CMD, CMD56) is described as:
>
>  GEN_CMD is the same as the single block read or write
>  commands (CMD24 or CMD17). The difference is that [...]
>  the data block is not a memory payload data but has a
>  vendor specific format and meaning.
>
>Thus this block must not be stored overwriting data block
>on underlying storage drive. Keep it in a dedicated
>'vendor_data[]' array.
>


I am reading the 4.3 spec, and it says:

  The bus transaction of the GEN_CMD is the same as the single block 
  read or write commands (CMD24 or CMD17). The difference is that the 
  argument denotes the direction of the data transfer (rather than the 
  address) and the data block is not a memory payload data but has a 
  vendor specific format and meaning.

The vendor here (qemu) does not support any functionality with CMD56. I 
think the correct approach would be to read zeros and discard writes, 
without storing anything and without changing data_offset (which is for 
`data` buffer)

What do you think?


>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
>v43: Do not re-use VMSTATE_UNUSED_V (danpb)
>v44: Use subsection (Luc)
>v45: Remove APP_READ_BLOCK/APP_WRITE_BLOCK macros
>---
> hw/sd/sd.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
>diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>index a08a452d81..5d58937dd4 100644
>--- a/hw/sd/sd.c
>+++ b/hw/sd/sd.c
>@@ -153,6 +153,8 @@ struct SDState {
>     uint32_t data_offset;
>     size_t data_size;
>     uint8_t data[512];
>+    uint8_t vendor_data[512];
>+
>     qemu_irq readonly_cb;
>     qemu_irq inserted_cb;
>     QEMUTimer *ocr_power_timer;
>@@ -719,6 +721,7 @@ static void sd_reset(DeviceState *dev)
>     sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
>     sd->wp_group_bits = sect;
>     sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
>+    memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data));
>     memset(sd->function_group, 0, sizeof(sd->function_group));
>     sd->erase_start = INVALID_ADDRESS;
>     sd->erase_end = INVALID_ADDRESS;
>@@ -793,6 +796,16 @@ static const VMStateDescription sd_ocr_vmstate = {
>     },
> };
> 
>+static const VMStateDescription sd_vendordata_vmstate = {
>+    .name = "sd-card/vendor_data-state",
>+    .version_id = 1,
>+    .minimum_version_id = 1,
>+    .fields = (const VMStateField[]) {
>+        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),
>+        VMSTATE_END_OF_LIST()
>+    },
>+};
>+
> static int sd_vmstate_pre_load(void *opaque)
> {
>     SDState *sd = opaque;
>@@ -840,6 +853,7 @@ static const VMStateDescription sd_vmstate = {
>     },
>     .subsections = (const VMStateDescription * const []) {
>         &sd_ocr_vmstate,
>+        &sd_vendordata_vmstate,
>         NULL
>     },
> };
>@@ -902,9 +916,6 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>     }
> }
> 
>-#define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
>-#define APP_WRITE_BLOCK(a, len)
>-
> static void sd_erase(SDState *sd)
> {
>     uint64_t erase_start = sd->erase_start;
>@@ -2187,9 +2198,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
>         break;
> 
>     case 56:  /* CMD56:  GEN_CMD */
>-        sd->data[sd->data_offset ++] = value;
>-        if (sd->data_offset >= sd->blk_len) {
>-            APP_WRITE_BLOCK(sd->data_start, sd->data_offset);
>+        sd->vendor_data[sd->data_offset++] = value;
>+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
>             sd->state = sd_transfer_state;
>         }
>         break;
>@@ -2261,12 +2271,11 @@ uint8_t sd_read_byte(SDState *sd)
>         break;
> 
>     case 56:  /* CMD56:  GEN_CMD */
>-        if (sd->data_offset == 0)
>-            APP_READ_BLOCK(sd->data_start, sd->blk_len);
>-        ret = sd->data[sd->data_offset ++];
>+        ret = sd->vendor_data[sd->data_offset++];
> 
>-        if (sd->data_offset >= sd->blk_len)
>+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
>             sd->state = sd_transfer_state;
>+        }
>         break;
> 
>     default:
>-- 
>2.41.0
>
>

Re: [PATCH v45 3/3] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

[Philippe Mathieu-Daudé]

On 3/7/24 14:24, Manos Pitsidianakis wrote:
> On Wed, 03 Jul 2024 11:59, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>> "General command" (GEN_CMD, CMD56) is described as:
>>
>>  GEN_CMD is the same as the single block read or write
>>  commands (CMD24 or CMD17). The difference is that [...]
>>  the data block is not a memory payload data but has a
>>  vendor specific format and meaning.
>>
>> Thus this block must not be stored overwriting data block
>> on underlying storage drive. Keep it in a dedicated
>> 'vendor_data[]' array.
>>
> 
> 
> I am reading the 4.3 spec, and it says:
> 
>   The bus transaction of the GEN_CMD is the same as the single block 
>   read or write commands (CMD24 or CMD17). The difference is that the 
>   argument denotes the direction of the data transfer (rather than the 
>   address) and the data block is not a memory payload data but has a 
>   vendor specific format and meaning.
> 
> The vendor here (qemu) does not support any functionality with CMD56. I 
> think the correct approach would be to read zeros and discard writes, 
> without storing anything and without changing data_offset (which is for 
> `data` buffer)
> 
> What do you think?

As Luc suggested in v42. Indeed simpler thus clever.

>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> v43: Do not re-use VMSTATE_UNUSED_V (danpb)
>> v44: Use subsection (Luc)
>> v45: Remove APP_READ_BLOCK/APP_WRITE_BLOCK macros
>> ---
>> hw/sd/sd.c | 29 +++++++++++++++++++----------
>> 1 file changed, 19 insertions(+), 10 deletions(-)