[PATCH 0/4] hw/sd/milkymist: Do not create SD card within the SDHCI controller

[PATCH 0/4] hw/sd/milkymist: Do not create SD card within the SDHCI controller

[Philippe Mathieu-Daudé]

SDHCI controllers provide a SD Bus to plug SD cards, but don't
come with SD card plugged in :)

This series move the SD card creation to the machine/board code.

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/lm32/milkymist: Un-inline milkymist_memcard_create()
  hw/lm32/milkymist: Comment to remember some IRQs lines are left
    unwired
  hw/sd/milkymist: Create the SDBus at init()
  hw/sd/milkymist: Do not create SD card within the SDHCI controller

 hw/lm32/milkymist-hw.h    | 11 --------
 hw/lm32/milkymist.c       | 25 +++++++++++++++++
 hw/sd/milkymist-memcard.c | 57 ++++++++++++++++++++++-----------------
 3 files changed, 58 insertions(+), 35 deletions(-)

-- 
2.21.3

[PATCH 1/4] hw/lm32/milkymist: Un-inline milkymist_memcard_create()

[Philippe Mathieu-Daudé]

As we will modify milkymist_memcard_create(), move it first
to the source file where it is used.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/lm32/milkymist-hw.h | 11 -----------
 hw/lm32/milkymist.c    | 11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/lm32/milkymist-hw.h b/hw/lm32/milkymist-hw.h
index 05e2c2a5a7..5dca5d52f5 100644
--- a/hw/lm32/milkymist-hw.h
+++ b/hw/lm32/milkymist-hw.h
@@ -31,17 +31,6 @@ static inline DeviceState *milkymist_hpdmc_create(hwaddr base)
     return dev;
 }
 
-static inline DeviceState *milkymist_memcard_create(hwaddr base)
-{
-    DeviceState *dev;
-
-    dev = qdev_new("milkymist-memcard");
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-
-    return dev;
-}
-
 static inline DeviceState *milkymist_vgafb_create(hwaddr base,
         uint32_t fb_offset, uint32_t fb_mask)
 {
diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
index 85913bb68b..469e3c4322 100644
--- a/hw/lm32/milkymist.c
+++ b/hw/lm32/milkymist.c
@@ -80,6 +80,17 @@ static void main_cpu_reset(void *opaque)
     env->deba = reset_info->flash_base;
 }
 
+static DeviceState *milkymist_memcard_create(hwaddr base)
+{
+    DeviceState *dev;
+
+    dev = qdev_new("milkymist-memcard");
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+
+    return dev;
+}
+
 static void
 milkymist_init(MachineState *machine)
 {
-- 
2.21.3

[PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Philippe Mathieu-Daudé]

The 'card is readonly' and 'card inserted' IRQs are not wired.
Add a comment in case someone know where to wire them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/lm32/milkymist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
index 469e3c4322..117973c967 100644
--- a/hw/lm32/milkymist.c
+++ b/hw/lm32/milkymist.c
@@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
     dev = qdev_new("milkymist-memcard");
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
 
     return dev;
 }
-- 
2.21.3

[PATCH 3/4] hw/sd/milkymist: Create the SDBus at init()

[Philippe Mathieu-Daudé]

We don't need to wait until realize() to create the SDBus,
create it in init() directly.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/milkymist-memcard.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index afdb8aa0c0..fb02309f07 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -261,6 +261,9 @@ static void milkymist_memcard_init(Object *obj)
     memory_region_init_io(&s->regs_region, OBJECT(s), &memcard_mmio_ops, s,
             "milkymist-memcard", R_MAX * 4);
     sysbus_init_mmio(dev, &s->regs_region);
+
+    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS,
+                        DEVICE(obj), "sd-bus");
 }
 
 static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
@@ -271,9 +274,6 @@ static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
     DriveInfo *dinfo;
     Error *err = NULL;
 
-    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS,
-                        dev, "sd-bus");
-
     /* Create and plug in the sd card */
     /* FIXME use a qdev drive property instead of drive_get_next() */
     dinfo = drive_get_next(IF_SD);
-- 
2.21.3

[PATCH 4/4] hw/sd/milkymist: Do not create SD card within the SDHCI controller

[Philippe Mathieu-Daudé]

SDHCI controllers provide a SD Bus to plug SD cards, but don't
come with SD card plugged in :) Let the machine/board object
create and plug the SD cards when required.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/lm32/milkymist.c       | 13 +++++++++
 hw/sd/milkymist-memcard.c | 55 +++++++++++++++++++++++----------------
 2 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
index 117973c967..8e2c68da5a 100644
--- a/hw/lm32/milkymist.c
+++ b/hw/lm32/milkymist.c
@@ -34,6 +34,7 @@
 #include "elf.h"
 #include "milkymist-hw.h"
 #include "hw/display/milkymist_tmu2.h"
+#include "hw/sd/sd.h"
 #include "lm32.h"
 #include "exec/address-spaces.h"
 #include "qemu/cutils.h"
@@ -83,12 +84,24 @@ static void main_cpu_reset(void *opaque)
 static DeviceState *milkymist_memcard_create(hwaddr base)
 {
     DeviceState *dev;
+    DriveInfo *dinfo;
 
     dev = qdev_new("milkymist-memcard");
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
     /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
 
+    dinfo = drive_get_next(IF_SD);
+    if (dinfo) {
+        DeviceState *card;
+
+        card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
+                               &error_fatal);
+    }
+
     return dev;
 }
 
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index fb02309f07..e9f5db5e22 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -66,6 +66,8 @@ enum {
 #define MILKYMIST_MEMCARD(obj) \
     OBJECT_CHECK(MilkymistMemcardState, (obj), TYPE_MILKYMIST_MEMCARD)
 
+#define TYPE_MILKYMIST_SDBUS "milkymist-sdbus"
+
 struct MilkymistMemcardState {
     SysBusDevice parent_obj;
 
@@ -253,6 +255,19 @@ static void milkymist_memcard_reset(DeviceState *d)
     }
 }
 
+static void milkymist_memcard_set_readonly(DeviceState *dev, bool level)
+{
+    qemu_log_mask(LOG_UNIMP,
+                  "milkymist_memcard: read-only mode not supported\n");
+}
+
+static void milkymist_memcard_set_inserted(DeviceState *dev, bool level)
+{
+    MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
+
+    s->enabled = !!level;
+}
+
 static void milkymist_memcard_init(Object *obj)
 {
     MilkymistMemcardState *s = MILKYMIST_MEMCARD(obj);
@@ -266,27 +281,6 @@ static void milkymist_memcard_init(Object *obj)
                         DEVICE(obj), "sd-bus");
 }
 
-static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
-{
-    MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
-    DeviceState *carddev;
-    BlockBackend *blk;
-    DriveInfo *dinfo;
-    Error *err = NULL;
-
-    /* Create and plug in the sd card */
-    /* FIXME use a qdev drive property instead of drive_get_next() */
-    dinfo = drive_get_next(IF_SD);
-    blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
-    carddev = qdev_new(TYPE_SD_CARD);
-    qdev_prop_set_drive(carddev, "drive", blk);
-    if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err)) {
-        error_propagate_prepend(errp, err, "failed to init SD card: %s");
-        return;
-    }
-    s->enabled = blk && blk_is_inserted(blk);
-}
-
 static const VMStateDescription vmstate_milkymist_memcard = {
     .name = "milkymist-memcard",
     .version_id = 1,
@@ -308,10 +302,9 @@ static void milkymist_memcard_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->realize = milkymist_memcard_realize;
     dc->reset = milkymist_memcard_reset;
     dc->vmsd = &vmstate_milkymist_memcard;
-    /* Reason: init() method uses drive_get_next() */
+    /* Reason: output IRQs should be wired up */
     dc->user_creatable = false;
 }
 
@@ -323,9 +316,25 @@ static const TypeInfo milkymist_memcard_info = {
     .class_init    = milkymist_memcard_class_init,
 };
 
+static void milkymist_sdbus_class_init(ObjectClass *klass, void *data)
+{
+    SDBusClass *sbc = SD_BUS_CLASS(klass);
+
+    sbc->set_inserted = milkymist_memcard_set_inserted;
+    sbc->set_readonly = milkymist_memcard_set_readonly;
+}
+
+static const TypeInfo milkymist_sdbus_info = {
+    .name = TYPE_MILKYMIST_SDBUS,
+    .parent = TYPE_SD_BUS,
+    .instance_size = sizeof(SDBus),
+    .class_init = milkymist_sdbus_class_init,
+};
+
 static void milkymist_memcard_register_types(void)
 {
     type_register_static(&milkymist_memcard_info);
+    type_register_static(&milkymist_sdbus_info);
 }
 
 type_init(milkymist_memcard_register_types)
-- 
2.21.3

Re: [PATCH 1/4] hw/lm32/milkymist: Un-inline milkymist_memcard_create()

[Alistair Francis]

On Sun, Jul 5, 2020 at 2:13 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> As we will modify milkymist_memcard_create(), move it first
> to the source file where it is used.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/lm32/milkymist-hw.h | 11 -----------
>  hw/lm32/milkymist.c    | 11 +++++++++++
>  2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/lm32/milkymist-hw.h b/hw/lm32/milkymist-hw.h
> index 05e2c2a5a7..5dca5d52f5 100644
> --- a/hw/lm32/milkymist-hw.h
> +++ b/hw/lm32/milkymist-hw.h
> @@ -31,17 +31,6 @@ static inline DeviceState *milkymist_hpdmc_create(hwaddr base)
>      return dev;
>  }
>
> -static inline DeviceState *milkymist_memcard_create(hwaddr base)
> -{
> -    DeviceState *dev;
> -
> -    dev = qdev_new("milkymist-memcard");
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> -
> -    return dev;
> -}
> -
>  static inline DeviceState *milkymist_vgafb_create(hwaddr base,
>          uint32_t fb_offset, uint32_t fb_mask)
>  {
> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
> index 85913bb68b..469e3c4322 100644
> --- a/hw/lm32/milkymist.c
> +++ b/hw/lm32/milkymist.c
> @@ -80,6 +80,17 @@ static void main_cpu_reset(void *opaque)
>      env->deba = reset_info->flash_base;
>  }
>
> +static DeviceState *milkymist_memcard_create(hwaddr base)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_new("milkymist-memcard");
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +
> +    return dev;
> +}
> +
>  static void
>  milkymist_init(MachineState *machine)
>  {
> --
> 2.21.3
>
>

Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Alistair Francis]

On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The 'card is readonly' and 'card inserted' IRQs are not wired.
> Add a comment in case someone know where to wire them.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I'm not convinced adding fixmes or todos in the code is the right
direction. It would be better to file bugs or use some other more
official tracking mechanism.

Alistair

> ---
>  hw/lm32/milkymist.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
> index 469e3c4322..117973c967 100644
> --- a/hw/lm32/milkymist.c
> +++ b/hw/lm32/milkymist.c
> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
>      dev = qdev_new("milkymist-memcard");
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
>
>      return dev;
>  }
> --
> 2.21.3
>
>

Re: [PATCH 3/4] hw/sd/milkymist: Create the SDBus at init()

[Alistair Francis]

On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> We don't need to wait until realize() to create the SDBus,
> create it in init() directly.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/sd/milkymist-memcard.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index afdb8aa0c0..fb02309f07 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -261,6 +261,9 @@ static void milkymist_memcard_init(Object *obj)
>      memory_region_init_io(&s->regs_region, OBJECT(s), &memcard_mmio_ops, s,
>              "milkymist-memcard", R_MAX * 4);
>      sysbus_init_mmio(dev, &s->regs_region);
> +
> +    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS,
> +                        DEVICE(obj), "sd-bus");
>  }
>
>  static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
> @@ -271,9 +274,6 @@ static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
>      DriveInfo *dinfo;
>      Error *err = NULL;
>
> -    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS,
> -                        dev, "sd-bus");
> -
>      /* Create and plug in the sd card */
>      /* FIXME use a qdev drive property instead of drive_get_next() */
>      dinfo = drive_get_next(IF_SD);
> --
> 2.21.3
>
>

Re: [PATCH 4/4] hw/sd/milkymist: Do not create SD card within the SDHCI controller

[Alistair Francis]

On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> SDHCI controllers provide a SD Bus to plug SD cards, but don't
> come with SD card plugged in :) Let the machine/board object
> create and plug the SD cards when required.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/lm32/milkymist.c       | 13 +++++++++
>  hw/sd/milkymist-memcard.c | 55 +++++++++++++++++++++++----------------
>  2 files changed, 45 insertions(+), 23 deletions(-)
>
> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
> index 117973c967..8e2c68da5a 100644
> --- a/hw/lm32/milkymist.c
> +++ b/hw/lm32/milkymist.c
> @@ -34,6 +34,7 @@
>  #include "elf.h"
>  #include "milkymist-hw.h"
>  #include "hw/display/milkymist_tmu2.h"
> +#include "hw/sd/sd.h"
>  #include "lm32.h"
>  #include "exec/address-spaces.h"
>  #include "qemu/cutils.h"
> @@ -83,12 +84,24 @@ static void main_cpu_reset(void *opaque)
>  static DeviceState *milkymist_memcard_create(hwaddr base)
>  {
>      DeviceState *dev;
> +    DriveInfo *dinfo;
>
>      dev = qdev_new("milkymist-memcard");
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>      /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
>
> +    dinfo = drive_get_next(IF_SD);
> +    if (dinfo) {
> +        DeviceState *card;
> +
> +        card = qdev_new(TYPE_SD_CARD);
> +        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
> +                                &error_fatal);
> +        qdev_realize_and_unref(card, qdev_get_child_bus(dev, "sd-bus"),
> +                               &error_fatal);
> +    }
> +
>      return dev;
>  }
>
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index fb02309f07..e9f5db5e22 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -66,6 +66,8 @@ enum {
>  #define MILKYMIST_MEMCARD(obj) \
>      OBJECT_CHECK(MilkymistMemcardState, (obj), TYPE_MILKYMIST_MEMCARD)
>
> +#define TYPE_MILKYMIST_SDBUS "milkymist-sdbus"
> +
>  struct MilkymistMemcardState {
>      SysBusDevice parent_obj;
>
> @@ -253,6 +255,19 @@ static void milkymist_memcard_reset(DeviceState *d)
>      }
>  }
>
> +static void milkymist_memcard_set_readonly(DeviceState *dev, bool level)
> +{
> +    qemu_log_mask(LOG_UNIMP,
> +                  "milkymist_memcard: read-only mode not supported\n");
> +}
> +
> +static void milkymist_memcard_set_inserted(DeviceState *dev, bool level)
> +{
> +    MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
> +
> +    s->enabled = !!level;
> +}
> +
>  static void milkymist_memcard_init(Object *obj)
>  {
>      MilkymistMemcardState *s = MILKYMIST_MEMCARD(obj);
> @@ -266,27 +281,6 @@ static void milkymist_memcard_init(Object *obj)
>                          DEVICE(obj), "sd-bus");
>  }
>
> -static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
> -{
> -    MilkymistMemcardState *s = MILKYMIST_MEMCARD(dev);
> -    DeviceState *carddev;
> -    BlockBackend *blk;
> -    DriveInfo *dinfo;
> -    Error *err = NULL;
> -
> -    /* Create and plug in the sd card */
> -    /* FIXME use a qdev drive property instead of drive_get_next() */
> -    dinfo = drive_get_next(IF_SD);
> -    blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
> -    carddev = qdev_new(TYPE_SD_CARD);
> -    qdev_prop_set_drive(carddev, "drive", blk);
> -    if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err)) {
> -        error_propagate_prepend(errp, err, "failed to init SD card: %s");
> -        return;
> -    }
> -    s->enabled = blk && blk_is_inserted(blk);
> -}
> -
>  static const VMStateDescription vmstate_milkymist_memcard = {
>      .name = "milkymist-memcard",
>      .version_id = 1,
> @@ -308,10 +302,9 @@ static void milkymist_memcard_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
> -    dc->realize = milkymist_memcard_realize;
>      dc->reset = milkymist_memcard_reset;
>      dc->vmsd = &vmstate_milkymist_memcard;
> -    /* Reason: init() method uses drive_get_next() */
> +    /* Reason: output IRQs should be wired up */
>      dc->user_creatable = false;
>  }
>
> @@ -323,9 +316,25 @@ static const TypeInfo milkymist_memcard_info = {
>      .class_init    = milkymist_memcard_class_init,
>  };
>
> +static void milkymist_sdbus_class_init(ObjectClass *klass, void *data)
> +{
> +    SDBusClass *sbc = SD_BUS_CLASS(klass);
> +
> +    sbc->set_inserted = milkymist_memcard_set_inserted;
> +    sbc->set_readonly = milkymist_memcard_set_readonly;
> +}
> +
> +static const TypeInfo milkymist_sdbus_info = {
> +    .name = TYPE_MILKYMIST_SDBUS,
> +    .parent = TYPE_SD_BUS,
> +    .instance_size = sizeof(SDBus),
> +    .class_init = milkymist_sdbus_class_init,
> +};
> +
>  static void milkymist_memcard_register_types(void)
>  {
>      type_register_static(&milkymist_memcard_info);
> +    type_register_static(&milkymist_sdbus_info);
>  }
>
>  type_init(milkymist_memcard_register_types)
> --
> 2.21.3
>
>

Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Philippe Mathieu-Daudé]

On 7/6/20 6:19 PM, Alistair Francis wrote:
> On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The 'card is readonly' and 'card inserted' IRQs are not wired.
>> Add a comment in case someone know where to wire them.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I'm not convinced adding fixmes or todos in the code is the right
> direction. It would be better to file bugs or use some other more
> official tracking mechanism.

This code is orphan :S

I'll fill a launchpad bug ticket.

OTOH we could also log UNIMP for lost IRQs (triggered but
no handler registered).

> 
> Alistair
> 
>> ---
>>  hw/lm32/milkymist.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
>> index 469e3c4322..117973c967 100644
>> --- a/hw/lm32/milkymist.c
>> +++ b/hw/lm32/milkymist.c
>> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
>>      dev = qdev_new("milkymist-memcard");
>>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
>>
>>      return dev;
>>  }
>> --
>> 2.21.3
>>
>>
> 

Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Alistair Francis]

On Mon, Jul 6, 2020 at 11:04 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/6/20 6:19 PM, Alistair Francis wrote:
> > On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> The 'card is readonly' and 'card inserted' IRQs are not wired.
> >> Add a comment in case someone know where to wire them.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > I'm not convinced adding fixmes or todos in the code is the right
> > direction. It would be better to file bugs or use some other more
> > official tracking mechanism.
>
> This code is orphan :S
>
> I'll fill a launchpad bug ticket.

I also mean in general (you have some other patches that add TODOs or FIXMEs).

>
> OTOH we could also log UNIMP for lost IRQs (triggered but
> no handler registered).

That would also work.

Alistair

>
> >
> > Alistair
> >
> >> ---
> >>  hw/lm32/milkymist.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
> >> index 469e3c4322..117973c967 100644
> >> --- a/hw/lm32/milkymist.c
> >> +++ b/hw/lm32/milkymist.c
> >> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
> >>      dev = qdev_new("milkymist-memcard");
> >>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> >>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> >> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
> >>
> >>      return dev;
> >>  }
> >> --
> >> 2.21.3
> >>
> >>
> >

Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Philippe Mathieu-Daudé]

On 7/6/20 8:32 PM, Alistair Francis wrote:
> On Mon, Jul 6, 2020 at 11:04 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 7/6/20 6:19 PM, Alistair Francis wrote:
>>> On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> The 'card is readonly' and 'card inserted' IRQs are not wired.
>>>> Add a comment in case someone know where to wire them.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> I'm not convinced adding fixmes or todos in the code is the right
>>> direction. It would be better to file bugs or use some other more
>>> official tracking mechanism.
>>
>> This code is orphan :S
>>
>> I'll fill a launchpad bug ticket.
> 
> I also mean in general (you have some other patches that add TODOs or FIXMEs).

Not all developers look at launchpad, while all of them read the code ;)

$ git grep -E '(TODO|FIXME)' | wc -l
1899

For orphan code, a comment in the code might be better to remember
the technical debt to the next developers interested to rework this
piece of code (I'd rather not trust they'll dig in the mailing list
archive and launchpad tickets while staring at the code).

> 
>>
>> OTOH we could also log UNIMP for lost IRQs (triggered but
>> no handler registered).
> 
> That would also work.
> 
> Alistair
> 
>>
>>>
>>> Alistair
>>>
>>>> ---
>>>>  hw/lm32/milkymist.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
>>>> index 469e3c4322..117973c967 100644
>>>> --- a/hw/lm32/milkymist.c
>>>> +++ b/hw/lm32/milkymist.c
>>>> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
>>>>      dev = qdev_new("milkymist-memcard");
>>>>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>>>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>>>> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
>>>>
>>>>      return dev;
>>>>  }
>>>> --
>>>> 2.21.3
>>>>
>>>>
>>>
> 

Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Alistair Francis]

On Mon, Jul 6, 2020 at 7:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/6/20 8:32 PM, Alistair Francis wrote:
> > On Mon, Jul 6, 2020 at 11:04 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> On 7/6/20 6:19 PM, Alistair Francis wrote:
> >>> On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>>>
> >>>> The 'card is readonly' and 'card inserted' IRQs are not wired.
> >>>> Add a comment in case someone know where to wire them.
> >>>>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>
> >>> I'm not convinced adding fixmes or todos in the code is the right
> >>> direction. It would be better to file bugs or use some other more
> >>> official tracking mechanism.
> >>
> >> This code is orphan :S
> >>
> >> I'll fill a launchpad bug ticket.
> >
> > I also mean in general (you have some other patches that add TODOs or FIXMEs).
>
> Not all developers look at launchpad, while all of them read the code ;)

Good point.

>
> $ git grep -E '(TODO|FIXME)' | wc -l
> 1899
>
> For orphan code, a comment in the code might be better to remember
> the technical debt to the next developers interested to rework this
> piece of code (I'd rather not trust they'll dig in the mailing list
> archive and launchpad tickets while staring at the code).

Agreed. I guess this is fine then. If possible/applicable a log
message would be more helpful.

Alistair

>
> >
> >>
> >> OTOH we could also log UNIMP for lost IRQs (triggered but
> >> no handler registered).
> >
> > That would also work.
> >
> > Alistair
> >
> >>
> >>>
> >>> Alistair
> >>>
> >>>> ---
> >>>>  hw/lm32/milkymist.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
> >>>> index 469e3c4322..117973c967 100644
> >>>> --- a/hw/lm32/milkymist.c
> >>>> +++ b/hw/lm32/milkymist.c
> >>>> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
> >>>>      dev = qdev_new("milkymist-memcard");
> >>>>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> >>>>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> >>>> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
> >>>>
> >>>>      return dev;
> >>>>  }
> >>>> --
> >>>> 2.21.3
> >>>>
> >>>>
> >>>
> >

Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Peter Maydell]

On Sun, 5 Jul 2020 at 22:10, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The 'card is readonly' and 'card inserted' IRQs are not wired.
> Add a comment in case someone know where to wire them.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/lm32/milkymist.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
> index 469e3c4322..117973c967 100644
> --- a/hw/lm32/milkymist.c
> +++ b/hw/lm32/milkymist.c
> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
>      dev = qdev_new("milkymist-memcard");
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */

It's possible that these lines are correctly not wired up
(ie that the hardware does not provide any kind of indication
of the r/o or insertion events). The milkymist mmc device is a
very simple one. AIUI the RTL for the board is on github if
anybody wants to go check.

thanks
-- PMM

Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Michael Walle]

Am 2020-07-07 18:30, schrieb Peter Maydell:
> On Sun, 5 Jul 2020 at 22:10, Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>> 
>> The 'card is readonly' and 'card inserted' IRQs are not wired.
>> Add a comment in case someone know where to wire them.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/lm32/milkymist.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
>> index 469e3c4322..117973c967 100644
>> --- a/hw/lm32/milkymist.c
>> +++ b/hw/lm32/milkymist.c
>> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr 
>> base)
>>      dev = qdev_new("milkymist-memcard");
>>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
> 
> It's possible that these lines are correctly not wired up
> (ie that the hardware does not provide any kind of indication
> of the r/o or insertion events). The milkymist mmc device is a
> very simple one. AIUI the RTL for the board is on github if
> anybody wants to go check.

That is correct, there are not indications nor are there any
interrupts.

-michael