[PATCH v5 0/8] Misc NPCM7XX patches

[PATCH v5 0/8] Misc NPCM7XX patches

[Hao Wu]

[NOTE: I'm reviving a bunch of patches that was in the process of
upstreaming a while ago but paused.]

This patch set contains a few bug fixes and I2C devices for some
NPCM7XX boards.

Patch 1~2 fix a problem that causes the SMBus module to behave
incorrectly when it's in FIFO mode and trying to receive more than
16 bytes at a time.

Patch 3 fixes a error in a register for ADC module.

Patch 4 makes the ADC input to be R/W instead of write only. It allows
a test system to read these via QMP and has no negative effect.

Patch 5 adds a new blockdev IF type IF_OTHER.

Patch 6 allows at24c_eeprom_init to take a bus as parameter so it can
be used by more use cases (e.g. behind an I2C mux.)

Patch 7 allows at24c_eeprom_init to take a drive as property, similar
to sdhci_attach_device().

Patch 8 uses the function defined in patch 5 to add the EEPROM and other
I2C devices for Quanta GBS board.

-- Changes since v4:
1. Add comments to patch 5.
2. Split patch 6 into 2 patches according to the feedback.  Each patch does it own task.

-- Changes since v3:
1. Add a new blockdev IF type IF_OTHER.
2. Use IF_OTHER instead of IF_NONE.

-- Changes since v2:
1. Dropped patch 7.
2. Drop an extra variable in patch 5.

-- Changes since v1:
1. Rewrote patch 5 to implement the function in NPCM7xx board file instead
   of the EEPROM device file.
2. Slightly modify patch 6 to adapt to the changes and QEMU comment style.
3. Squash patch 7 into patch 5 to make it compile.
4. Add a new patch 7.

Hao Wu (7):
  hw/i2c: Clear ACK bit in NPCM7xx SMBus module
  hw/i2c: Read FIFO during RXF_CTL change in NPCM7XX SMBus
  hw/adc: Fix CONV bit in NPCM7XX ADC CON register
  hw/adc: Make adci[*] R/W in NPCM7XX ADC
  blockdev: Add a new IF type IF_OTHER
  hw/arm: npcm8xx_boards: EEPROMs can take bus as parameter
  hw/arm: Set drive property for at24c eeprom

Patrick Venture (1):
  hw/arm: quanta-gbs-bmc add i2c devices

 blockdev.c                     |   4 +-
 hw/adc/npcm7xx_adc.c           |   4 +-
 hw/arm/npcm7xx_boards.c        | 102 ++++++++++++++++++++-------------
 hw/i2c/npcm7xx_smbus.c         |   8 +--
 include/sysemu/blockdev.h      |   1 +
 tests/qtest/npcm7xx_adc-test.c |   2 +-
 6 files changed, 73 insertions(+), 48 deletions(-)

-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v5 1/8] hw/i2c: Clear ACK bit in NPCM7xx SMBus module

[Hao Wu]

The ACK bit in NPCM7XX SMBus module should be cleared each time it
sends out a NACK signal. This patch fixes the bug that it fails to
do so.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Titus Rwantare <titusr@google.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i2c/npcm7xx_smbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
index e7e0ba66fe..f18e311556 100644
--- a/hw/i2c/npcm7xx_smbus.c
+++ b/hw/i2c/npcm7xx_smbus.c
@@ -270,7 +270,7 @@ static void npcm7xx_smbus_recv_byte(NPCM7xxSMBusState *s)
     if (s->st & NPCM7XX_SMBCTL1_ACK) {
         trace_npcm7xx_smbus_nack(DEVICE(s)->canonical_path);
         i2c_nack(s->bus);
-        s->st &= NPCM7XX_SMBCTL1_ACK;
+        s->st &= ~NPCM7XX_SMBCTL1_ACK;
     }
     trace_npcm7xx_smbus_recv_byte((DEVICE(s)->canonical_path), s->sda);
     npcm7xx_smbus_update_irq(s);
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v5 2/8] hw/i2c: Read FIFO during RXF_CTL change in NPCM7XX SMBus

[Hao Wu]

Originally we read in from SMBus when RXF_STS is cleared. However,
the driver clears RXF_STS before setting RXF_CTL, causing the SM bus
module to read incorrect amount of bytes in FIFO mode when the number
of bytes read changed. This patch fixes this issue.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Titus Rwantare <titusr@google.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/npcm7xx_smbus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
index f18e311556..1435daea94 100644
--- a/hw/i2c/npcm7xx_smbus.c
+++ b/hw/i2c/npcm7xx_smbus.c
@@ -637,9 +637,6 @@ static void npcm7xx_smbus_write_rxf_sts(NPCM7xxSMBusState *s, uint8_t value)
 {
     if (value & NPCM7XX_SMBRXF_STS_RX_THST) {
         s->rxf_sts &= ~NPCM7XX_SMBRXF_STS_RX_THST;
-        if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
-            npcm7xx_smbus_recv_fifo(s);
-        }
     }
 }
 
@@ -651,6 +648,9 @@ static void npcm7xx_smbus_write_rxf_ctl(NPCM7xxSMBusState *s, uint8_t value)
         new_ctl = KEEP_OLD_BIT(s->rxf_ctl, new_ctl, NPCM7XX_SMBRXF_CTL_LAST);
     }
     s->rxf_ctl = new_ctl;
+    if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
+        npcm7xx_smbus_recv_fifo(s);
+    }
 }
 
 static uint64_t npcm7xx_smbus_read(void *opaque, hwaddr offset, unsigned size)
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v5 2/8] hw/i2c: Read FIFO during RXF_CTL change in NPCM7XX SMBus

[Peter Maydell]

On Thu, 14 Jul 2022 at 19:28, Hao Wu <wuhaotsh@google.com> wrote:
>
> Originally we read in from SMBus when RXF_STS is cleared. However,
> the driver clears RXF_STS before setting RXF_CTL, causing the SM bus
> module to read incorrect amount of bytes in FIFO mode when the number
> of bytes read changed. This patch fixes this issue.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Titus Rwantare <titusr@google.com>
> Acked-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/i2c/npcm7xx_smbus.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
> index f18e311556..1435daea94 100644
> --- a/hw/i2c/npcm7xx_smbus.c
> +++ b/hw/i2c/npcm7xx_smbus.c
> @@ -637,9 +637,6 @@ static void npcm7xx_smbus_write_rxf_sts(NPCM7xxSMBusState *s, uint8_t value)
>  {
>      if (value & NPCM7XX_SMBRXF_STS_RX_THST) {
>          s->rxf_sts &= ~NPCM7XX_SMBRXF_STS_RX_THST;
> -        if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
> -            npcm7xx_smbus_recv_fifo(s);
> -        }
>      }
>  }
>
> @@ -651,6 +648,9 @@ static void npcm7xx_smbus_write_rxf_ctl(NPCM7xxSMBusState *s, uint8_t value)
>          new_ctl = KEEP_OLD_BIT(s->rxf_ctl, new_ctl, NPCM7XX_SMBRXF_CTL_LAST);
>      }
>      s->rxf_ctl = new_ctl;
> +    if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) {
> +        npcm7xx_smbus_recv_fifo(s);
> +    }
>  }

I don't know anything about this hardware, but this looks a bit odd.
Why should we care what order the driver does the register operations
in? Do we really want to read new fifo data regardless of what value
the driver writes to RXF_CTL ? Should the logic actually be "if the
new device register state is <whatever> then read fifo data", and
checked in both places ?

thanks
-- PMM

[PATCH v5 3/8] hw/adc: Fix CONV bit in NPCM7XX ADC CON register

[Hao Wu]

The correct bit for the CONV bit in NPCM7XX ADC is bit 13. This patch
fixes that in the module, and also lower the IRQ when the guest
is done handling an interrupt event from the ADC module.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Patrick Venture<venture@google.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/adc/npcm7xx_adc.c           | 2 +-
 tests/qtest/npcm7xx_adc-test.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/adc/npcm7xx_adc.c b/hw/adc/npcm7xx_adc.c
index 0f0a9f63e2..47fb9e5f74 100644
--- a/hw/adc/npcm7xx_adc.c
+++ b/hw/adc/npcm7xx_adc.c
@@ -36,7 +36,7 @@ REG32(NPCM7XX_ADC_DATA, 0x4)
 #define NPCM7XX_ADC_CON_INT     BIT(18)
 #define NPCM7XX_ADC_CON_EN      BIT(17)
 #define NPCM7XX_ADC_CON_RST     BIT(16)
-#define NPCM7XX_ADC_CON_CONV    BIT(14)
+#define NPCM7XX_ADC_CON_CONV    BIT(13)
 #define NPCM7XX_ADC_CON_DIV(rv) extract32(rv, 1, 8)
 
 #define NPCM7XX_ADC_MAX_RESULT      1023
diff --git a/tests/qtest/npcm7xx_adc-test.c b/tests/qtest/npcm7xx_adc-test.c
index 3fa6d9ece0..8048044d28 100644
--- a/tests/qtest/npcm7xx_adc-test.c
+++ b/tests/qtest/npcm7xx_adc-test.c
@@ -50,7 +50,7 @@
 #define CON_INT     BIT(18)
 #define CON_EN      BIT(17)
 #define CON_RST     BIT(16)
-#define CON_CONV    BIT(14)
+#define CON_CONV    BIT(13)
 #define CON_DIV(rv) extract32(rv, 1, 8)
 
 #define FST_RDST    BIT(1)
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v5 4/8] hw/adc: Make adci[*] R/W in NPCM7XX ADC

[Hao Wu]

Our sensor test requires both reading and writing from a sensor's
QOM property. So we need to make the input of ADC module R/W instead
of write only for that to work.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Titus Rwantare <titusr@google.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/adc/npcm7xx_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/adc/npcm7xx_adc.c b/hw/adc/npcm7xx_adc.c
index 47fb9e5f74..bc6f3f55e6 100644
--- a/hw/adc/npcm7xx_adc.c
+++ b/hw/adc/npcm7xx_adc.c
@@ -242,7 +242,7 @@ static void npcm7xx_adc_init(Object *obj)
 
     for (i = 0; i < NPCM7XX_ADC_NUM_INPUTS; ++i) {
         object_property_add_uint32_ptr(obj, "adci[*]",
-                &s->adci[i], OBJ_PROP_FLAG_WRITE);
+                &s->adci[i], OBJ_PROP_FLAG_READWRITE);
     }
     object_property_add_uint32_ptr(obj, "vref",
             &s->vref, OBJ_PROP_FLAG_WRITE);
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Hao Wu]

This type is used to represent block devs that are not suitable to
be represented by other existing types.

A sample use is to represent an at24c eeprom device defined in
hw/nvram/eeprom_at24c.c. The block device can be used to contain the
content of the said eeprom device.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 blockdev.c                | 4 +++-
 include/sysemu/blockdev.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 9230888e34..befd69ac5f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -82,6 +82,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_MTD] = "mtd",
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
+    [IF_OTHER] = "other",
     [IF_XEN] = "xen",
 };
 
@@ -726,7 +727,8 @@ QemuOptsList qemu_legacy_drive_opts = {
         },{
             .name = "if",
             .type = QEMU_OPT_STRING,
-            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
+            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio,"
+                    " other)",
         },{
             .name = "file",
             .type = QEMU_OPT_STRING,
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 3211b16513..d9dd5af291 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -21,6 +21,7 @@ typedef enum {
      */
     IF_NONE = 0,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_OTHER,
     IF_COUNT
 } BlockInterfaceType;
 
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Markus Armbruster]

Hao Wu <wuhaotsh@google.com> writes:

> This type is used to represent block devs that are not suitable to
> be represented by other existing types.
>
> A sample use is to represent an at24c eeprom device defined in
> hw/nvram/eeprom_at24c.c. The block device can be used to contain the
> content of the said eeprom device.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>

Let me add a bit more history in the hope of helping other reviewers.

v3 of this series[1] used IF_NONE for the EEPROM device.

Peter Maydell questioned that[2], and we concluded it's wrong.  I wrote

    [A] board can use any traditional interface type.  If none of them
    matches, and common decency prevents use of a non-matching one,
    invent a new one.  We could do IF_OTHER.

Thomas Huth cleaned up the existing abuse of IF_NONE to use IF_PFLASH
instead, in commit 6b717a8d44:

    hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

    Configuring a drive with "if=none" is meant for creation of a backend
    only, it should not get automatically assigned to a device frontend.
    Use "if=pflash" for the One-Time-Programmable device instead (like
    it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).

    Since the old way of configuring the device has already been published
    with the previous QEMU versions, we cannot remove this immediately, but
    have to deprecate it and support it for at least two more releases.

    Signed-off-by: Thomas Huth <thuth@redhat.com>
    Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>
    Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
    Message-id: 20211119102549.217755-1-thuth@redhat.com
    Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

An OTP device isn't really a parallel flash, and neither are eFuses.
More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
other interface types, too.

This patch introduces IF_OTHER.  The patch after next uses it for an
EEPROM device.

Do we want IF_OTHER?

If no, I guess we get to abuse IF_PFLASH some more.

If yes, I guess we should use IF_PFLASH only for actual parallel flash
memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
be worth the trouble, though.

Thoughts?

> ---
>  blockdev.c                | 4 +++-
>  include/sysemu/blockdev.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9230888e34..befd69ac5f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -82,6 +82,7 @@ static const char *const if_name[IF_COUNT] = {
>      [IF_MTD] = "mtd",
>      [IF_SD] = "sd",
>      [IF_VIRTIO] = "virtio",
> +    [IF_OTHER] = "other",
>      [IF_XEN] = "xen",
>  };
>  
> @@ -726,7 +727,8 @@ QemuOptsList qemu_legacy_drive_opts = {
>          },{
>              .name = "if",
>              .type = QEMU_OPT_STRING,
> -            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> +            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio,"
> +                    " other)",
>          },{
>              .name = "file",
>              .type = QEMU_OPT_STRING,
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 3211b16513..d9dd5af291 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -21,6 +21,7 @@ typedef enum {
>       */
>      IF_NONE = 0,
>      IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +    IF_OTHER,
>      IF_COUNT
>  } BlockInterfaceType;


[1] https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00372.html

[2] Subject: does drive_get_next(IF_NONE) make sense?
    Message-ID: <CAFEAcA9zmPds0+jHm8VY465XEhK6bbVPd+nDob1ruRPaHOua_Q@mail.gmail.com>
    https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00710.html

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Thomas Huth]

On 18/07/2022 11.49, Markus Armbruster wrote:
[...]
> An OTP device isn't really a parallel flash, and neither are eFuses.
> More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> other interface types, too.
> 
> This patch introduces IF_OTHER.  The patch after next uses it for an
> EEPROM device.
> 
> Do we want IF_OTHER?
> 
> If no, I guess we get to abuse IF_PFLASH some more.
> 
> If yes, I guess we should use IF_PFLASH only for actual parallel flash
> memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> be worth the trouble, though.
> 
> Thoughts?

Maybe we should simply rename IF_PFLASH to IF_EPROM or IF_FLASH to make it 
clear that it is not only about parallel flashes anymore? Just my 0.02 €.

  Thomas

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Kevin Wolf]

Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> Hao Wu <wuhaotsh@google.com> writes:
> 
> > This type is used to represent block devs that are not suitable to
> > be represented by other existing types.
> >
> > A sample use is to represent an at24c eeprom device defined in
> > hw/nvram/eeprom_at24c.c. The block device can be used to contain the
> > content of the said eeprom device.
> >
> > Signed-off-by: Hao Wu <wuhaotsh@google.com>
> 
> Let me add a bit more history in the hope of helping other reviewers.
> 
> v3 of this series[1] used IF_NONE for the EEPROM device.
> 
> Peter Maydell questioned that[2], and we concluded it's wrong.  I wrote
> 
>     [A] board can use any traditional interface type.  If none of them
>     matches, and common decency prevents use of a non-matching one,
>     invent a new one.  We could do IF_OTHER.
> 
> Thomas Huth cleaned up the existing abuse of IF_NONE to use IF_PFLASH
> instead, in commit 6b717a8d44:
> 
>     hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE
> 
>     Configuring a drive with "if=none" is meant for creation of a backend
>     only, it should not get automatically assigned to a device frontend.
>     Use "if=pflash" for the One-Time-Programmable device instead (like
>     it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
> 
>     Since the old way of configuring the device has already been published
>     with the previous QEMU versions, we cannot remove this immediately, but
>     have to deprecate it and support it for at least two more releases.
> 
>     Signed-off-by: Thomas Huth <thuth@redhat.com>
>     Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>     Reviewed-by: Markus Armbruster <armbru@redhat.com>
>     Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>     Message-id: 20211119102549.217755-1-thuth@redhat.com
>     Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> 
> An OTP device isn't really a parallel flash, and neither are eFuses.
> More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> other interface types, too.
> 
> This patch introduces IF_OTHER.  The patch after next uses it for an
> EEPROM device.
> 
> Do we want IF_OTHER?

What would the semantics even be? Any block device that doesn't pick up
a different category may pick up IF_OTHER backends?

It certainly feels like a strange interface to ask for "other" disk and
then getting as surprise what this other thing might be. It's
essentially the same as having an explicit '-device other', and I
suppose most people would find that strange.

> If no, I guess we get to abuse IF_PFLASH some more.
> 
> If yes, I guess we should use IF_PFLASH only for actual parallel flash
> memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> be worth the trouble, though.
> 
> Thoughts?

If the existing types aren't good enough (I don't have an opinion on
whether IF_PFLASH is a good match), let's add a new one. But a specific
new one, not just "other".

Kevin

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Peter Maydell]

On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> > An OTP device isn't really a parallel flash, and neither are eFuses.
> > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> > other interface types, too.
> >
> > This patch introduces IF_OTHER.  The patch after next uses it for an
> > EEPROM device.
> >
> > Do we want IF_OTHER?
>
> What would the semantics even be? Any block device that doesn't pick up
> a different category may pick up IF_OTHER backends?
>
> It certainly feels like a strange interface to ask for "other" disk and
> then getting as surprise what this other thing might be. It's
> essentially the same as having an explicit '-device other', and I
> suppose most people would find that strange.
>
> > If no, I guess we get to abuse IF_PFLASH some more.
> >
> > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> > be worth the trouble, though.
> >
> > Thoughts?
>
> If the existing types aren't good enough (I don't have an opinion on
> whether IF_PFLASH is a good match), let's add a new one. But a specific
> new one, not just "other".

I think the common thread is "this isn't what anybody actually thinks
of as being a 'disk', but we would like to back it with a block device
anyway". That can cover a fair range of possibilities...

thanks
-- PMM

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Kevin Wolf]

Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> > > other interface types, too.
> > >
> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> > > EEPROM device.
> > >
> > > Do we want IF_OTHER?
> >
> > What would the semantics even be? Any block device that doesn't pick up
> > a different category may pick up IF_OTHER backends?
> >
> > It certainly feels like a strange interface to ask for "other" disk and
> > then getting as surprise what this other thing might be. It's
> > essentially the same as having an explicit '-device other', and I
> > suppose most people would find that strange.
> >
> > > If no, I guess we get to abuse IF_PFLASH some more.
> > >
> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> > > be worth the trouble, though.
> > >
> > > Thoughts?
> >
> > If the existing types aren't good enough (I don't have an opinion on
> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> > new one, not just "other".
> 
> I think the common thread is "this isn't what anybody actually thinks
> of as being a 'disk', but we would like to back it with a block device
> anyway". That can cover a fair range of possibilities...

How confident are we that no board will ever have two devices of this
kind?

As long as every board has at most one, if=other is a bad user interface
in terms of descriptiveness, but still more or less workable as long as
you know what it means for the specific board you use.

But if you have more than one device, it becomes hard to predict which
device gets which backend - it depends on the initialisation order in
the code then, and I'm pretty sure that this isn't something that should
have significance in external interfaces and therefore become a stable
API.

Kevin

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Peter Maydell]

On Thu, 28 Jul 2022 at 14:30, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> > On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> > > If the existing types aren't good enough (I don't have an opinion on
> > > whether IF_PFLASH is a good match), let's add a new one. But a specific
> > > new one, not just "other".
> >
> > I think the common thread is "this isn't what anybody actually thinks
> > of as being a 'disk', but we would like to back it with a block device
> > anyway". That can cover a fair range of possibilities...
>
> How confident are we that no board will ever have two devices of this
> kind?
>
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.

Extremely non-confident, but that holds for all these things,
unless we add new IF_* for every possible new thing:
 IF_PFLASH
 IF_EEPROM
 IF_EFUSE
 IF_BBRAM
 ...
?

thanks
-- PMM

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Cédric Le Goater]

On 7/28/22 15:29, Kevin Wolf wrote:
> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
>> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
>>>
>>> Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
>>>> An OTP device isn't really a parallel flash, and neither are eFuses.
>>>> More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
>>>> other interface types, too.
>>>>
>>>> This patch introduces IF_OTHER.  The patch after next uses it for an
>>>> EEPROM device.
>>>>
>>>> Do we want IF_OTHER?
>>>
>>> What would the semantics even be? Any block device that doesn't pick up
>>> a different category may pick up IF_OTHER backends?
>>>
>>> It certainly feels like a strange interface to ask for "other" disk and
>>> then getting as surprise what this other thing might be. It's
>>> essentially the same as having an explicit '-device other', and I
>>> suppose most people would find that strange.
>>>
>>>> If no, I guess we get to abuse IF_PFLASH some more.
>>>>
>>>> If yes, I guess we should use IF_PFLASH only for actual parallel flash
>>>> memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
>>>> be worth the trouble, though.
>>>>
>>>> Thoughts?
>>>
>>> If the existing types aren't good enough (I don't have an opinion on
>>> whether IF_PFLASH is a good match), let's add a new one. But a specific
>>> new one, not just "other".
>>
>> I think the common thread is "this isn't what anybody actually thinks
>> of as being a 'disk', but we would like to back it with a block device
>> anyway". That can cover a fair range of possibilities...
> 
> How confident are we that no board will ever have two devices of this
> kind?

The BMC machines have a lot of eeproms.

> 
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.
> 
> But if you have more than one device, it becomes hard to predict which
> device gets which backend - it depends on the initialisation order in
> the code then, and I'm pretty sure that this isn't something that should
> have significance in external interfaces and therefore become a stable
> API.

Can't we use the drive index ?


There has been various attempts to solve this problem for the Aspeed
machines also. See below. May be we should introduce and IF_EEPROM for
the purpose.

Thanks,

C.

[PATCH v2] aspeed: qcom: add block backed FRU devices
https://www.mail-archive.com/qemu-devel@nongnu.org/msg900485.html

[PATCH] aspeed: Enable backend file for eeprom
http://patchwork.ozlabs.org/project/qemu-devel/patch/20220728061228.152704-1-wangzhiqiang02@inspur.com/

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
>> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
>> >
>> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
>> > > An OTP device isn't really a parallel flash, and neither are eFuses.
>> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
>> > > other interface types, too.
>> > >
>> > > This patch introduces IF_OTHER.  The patch after next uses it for an
>> > > EEPROM device.
>> > >
>> > > Do we want IF_OTHER?
>> >
>> > What would the semantics even be? Any block device that doesn't pick up
>> > a different category may pick up IF_OTHER backends?
>> >
>> > It certainly feels like a strange interface to ask for "other" disk and
>> > then getting as surprise what this other thing might be. It's
>> > essentially the same as having an explicit '-device other', and I
>> > suppose most people would find that strange.
>> >
>> > > If no, I guess we get to abuse IF_PFLASH some more.
>> > >
>> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
>> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
>> > > be worth the trouble, though.
>> > >
>> > > Thoughts?
>> >
>> > If the existing types aren't good enough (I don't have an opinion on
>> > whether IF_PFLASH is a good match), let's add a new one. But a specific
>> > new one, not just "other".
>> 
>> I think the common thread is "this isn't what anybody actually thinks
>> of as being a 'disk', but we would like to back it with a block device
>> anyway". That can cover a fair range of possibilities...
>
> How confident are we that no board will ever have two devices of this
> kind?
>
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.
>
> But if you have more than one device, it becomes hard to predict which
> device gets which backend - it depends on the initialisation order in
> the code then,

Really?  Board code should use IF_OTHER devices just like it uses the
other interface types, namely connecting each frontend device to a
backend device with a well-known and fixed interface type and index (or
bus and unit instead, where appropriate).

>                and I'm pretty sure that this isn't something that should
> have significance in external interfaces and therefore become a stable
> API.

I agree that "implied by execution order" is a bad idea: commit
95fd260f0a "blockdev: Drop unused drive_get_next()".

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Peter Maydell]

On Thu, 28 Jul 2022 at 15:50, Markus Armbruster <armbru@redhat.com> wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> >
> > But if you have more than one device, it becomes hard to predict which
> > device gets which backend - it depends on the initialisation order in
> > the code then,
>
> Really?  Board code should use IF_OTHER devices just like it uses the
> other interface types, namely connecting each frontend device to a
> backend device with a well-known and fixed interface type and index (or
> bus and unit instead, where appropriate).

I think part of the problem is that unlike the typical disk
interface, where there is some idea of bus-and-unit-number or
index number that it makes sense to expose to users, these
"miscellaneous storage" devices don't have any particular index
concept -- in the real hardware there are just a random set of
devices that are connected in various places. So you're requiring
users to look up the documentation for "index 0 is this eeprom,
index 1 is that other eeprom, index 2 is ...".

thanks
-- PMM

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 28 Jul 2022 at 15:50, Markus Armbruster <armbru@redhat.com> wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> >
>> > But if you have more than one device, it becomes hard to predict which
>> > device gets which backend - it depends on the initialisation order in
>> > the code then,
>>
>> Really?  Board code should use IF_OTHER devices just like it uses the
>> other interface types, namely connecting each frontend device to a
>> backend device with a well-known and fixed interface type and index (or
>> bus and unit instead, where appropriate).
>
> I think part of the problem is that unlike the typical disk
> interface, where there is some idea of bus-and-unit-number or
> index number that it makes sense to expose to users, these
> "miscellaneous storage" devices don't have any particular index
> concept -- in the real hardware there are just a random set of
> devices that are connected in various places. So you're requiring
> users to look up the documentation for "index 0 is this eeprom,
> index 1 is that other eeprom, index 2 is ...".

"Unit number on a bus" makes perfect sense for SCSI and PATA.  For SATA,
the only valid unit number is 0, which may or may not make sense to
users.  Not a problem in practice, though.

Bus numbers are arbitrary, though.  Harmless enough when you have to
deal only with very few of them, e.g. a single SCSI HBA (one bus, number
0), a single PATA HBA (two buses, number 0 and 1), a single SATA HBA
(typically six buses, numbers 0..5).

For anything else, we use "index" rather than "bus" and "unit", and the
indexes are completely arbitrary.  Again, harmless enough when you have
to deal only with a few of each interface type.

*Names* rather than arbitrary index or bus numbers would arguably be a
better interface.

Nothing of this is new with IF_OTHER.

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Kevin Wolf]

Am 28.07.2022 um 16:50 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> >> >
> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> >> > > other interface types, too.
> >> > >
> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> >> > > EEPROM device.
> >> > >
> >> > > Do we want IF_OTHER?
> >> >
> >> > What would the semantics even be? Any block device that doesn't pick up
> >> > a different category may pick up IF_OTHER backends?
> >> >
> >> > It certainly feels like a strange interface to ask for "other" disk and
> >> > then getting as surprise what this other thing might be. It's
> >> > essentially the same as having an explicit '-device other', and I
> >> > suppose most people would find that strange.
> >> >
> >> > > If no, I guess we get to abuse IF_PFLASH some more.
> >> > >
> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> >> > > be worth the trouble, though.
> >> > >
> >> > > Thoughts?
> >> >
> >> > If the existing types aren't good enough (I don't have an opinion on
> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> >> > new one, not just "other".
> >> 
> >> I think the common thread is "this isn't what anybody actually thinks
> >> of as being a 'disk', but we would like to back it with a block device
> >> anyway". That can cover a fair range of possibilities...
> >
> > How confident are we that no board will ever have two devices of this
> > kind?
> >
> > As long as every board has at most one, if=other is a bad user interface
> > in terms of descriptiveness, but still more or less workable as long as
> > you know what it means for the specific board you use.
> >
> > But if you have more than one device, it becomes hard to predict which
> > device gets which backend - it depends on the initialisation order in
> > the code then,
> 
> Really?  Board code should use IF_OTHER devices just like it uses the
> other interface types, namely connecting each frontend device to a
> backend device with a well-known and fixed interface type and index (or
> bus and unit instead, where appropriate).
> 
> >                and I'm pretty sure that this isn't something that should
> > have significance in external interfaces and therefore become a stable
> > API.
> 
> I agree that "implied by execution order" is a bad idea: commit
> 95fd260f0a "blockdev: Drop unused drive_get_next()".

Ah good, I was indeed thinking of something drive_get_next()-like.

In case the board works with explicit indices, the situation is not as
bad as I was afraid. It will certainly be workable (even if not obvious)
for any boards that have a fixed number of devices with block backends,
which should cover everything we're intending to cover here.

I still consider if=other a bad user interface because what it means is
completely opaque, but if that's okay for you in your board, who am I to
object.

(Of course, the real solution would be having a generic way to set qdev
properties for on-board devices. I'm not expecting that we're getting
this anytime soon, though.)

Kevin

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Daniel P. Berrangé]

On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> > > other interface types, too.
> > >
> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> > > EEPROM device.
> > >
> > > Do we want IF_OTHER?
> >
> > What would the semantics even be? Any block device that doesn't pick up
> > a different category may pick up IF_OTHER backends?
> >
> > It certainly feels like a strange interface to ask for "other" disk and
> > then getting as surprise what this other thing might be. It's
> > essentially the same as having an explicit '-device other', and I
> > suppose most people would find that strange.
> >
> > > If no, I guess we get to abuse IF_PFLASH some more.
> > >
> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> > > be worth the trouble, though.
> > >
> > > Thoughts?
> >
> > If the existing types aren't good enough (I don't have an opinion on
> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> > new one, not just "other".
> 
> I think the common thread is "this isn't what anybody actually thinks
> of as being a 'disk', but we would like to back it with a block device
> anyway". That can cover a fair range of possibilities...

Given that, do we even want/have to use -drive for this ?    We can use
-blockdev for the backend and reference that from any -device we want
to create, and leave -drive out of the picture entirely

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
>> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
>> >
>> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
>> > > An OTP device isn't really a parallel flash, and neither are eFuses.
>> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
>> > > other interface types, too.
>> > >
>> > > This patch introduces IF_OTHER.  The patch after next uses it for an
>> > > EEPROM device.
>> > >
>> > > Do we want IF_OTHER?
>> >
>> > What would the semantics even be? Any block device that doesn't pick up
>> > a different category may pick up IF_OTHER backends?
>> >
>> > It certainly feels like a strange interface to ask for "other" disk and
>> > then getting as surprise what this other thing might be. It's
>> > essentially the same as having an explicit '-device other', and I
>> > suppose most people would find that strange.
>> >
>> > > If no, I guess we get to abuse IF_PFLASH some more.
>> > >
>> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
>> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
>> > > be worth the trouble, though.
>> > >
>> > > Thoughts?
>> >
>> > If the existing types aren't good enough (I don't have an opinion on
>> > whether IF_PFLASH is a good match), let's add a new one. But a specific
>> > new one, not just "other".
>> 
>> I think the common thread is "this isn't what anybody actually thinks
>> of as being a 'disk', but we would like to back it with a block device
>> anyway". That can cover a fair range of possibilities...
>
> Given that, do we even want/have to use -drive for this ?    We can use
> -blockdev for the backend and reference that from any -device we want
> to create, and leave -drive out of the picture entirely

-drive is our only means to configure onboard devices.

We've talked about better means a few times, but no conclusions.  I can
dig up pointers, if you're interested.

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Daniel P. Berrangé]

On Thu, Aug 04, 2022 at 04:56:15PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> >> >
> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> >> > > other interface types, too.
> >> > >
> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> >> > > EEPROM device.
> >> > >
> >> > > Do we want IF_OTHER?
> >> >
> >> > What would the semantics even be? Any block device that doesn't pick up
> >> > a different category may pick up IF_OTHER backends?
> >> >
> >> > It certainly feels like a strange interface to ask for "other" disk and
> >> > then getting as surprise what this other thing might be. It's
> >> > essentially the same as having an explicit '-device other', and I
> >> > suppose most people would find that strange.
> >> >
> >> > > If no, I guess we get to abuse IF_PFLASH some more.
> >> > >
> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> >> > > be worth the trouble, though.
> >> > >
> >> > > Thoughts?
> >> >
> >> > If the existing types aren't good enough (I don't have an opinion on
> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> >> > new one, not just "other".
> >> 
> >> I think the common thread is "this isn't what anybody actually thinks
> >> of as being a 'disk', but we would like to back it with a block device
> >> anyway". That can cover a fair range of possibilities...
> >
> > Given that, do we even want/have to use -drive for this ?    We can use
> > -blockdev for the backend and reference that from any -device we want
> > to create, and leave -drive out of the picture entirely
> 
> -drive is our only means to configure onboard devices.
> 
> We've talked about better means a few times, but no conclusions.  I can
> dig up pointers, if you're interested.

For onboard pflash with x86, we've just got properties against the
machine that we can point to a blockdev.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Aug 04, 2022 at 04:56:15PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
>> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
>> >> >
>> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
>> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
>> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
>> >> > > other interface types, too.
>> >> > >
>> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
>> >> > > EEPROM device.
>> >> > >
>> >> > > Do we want IF_OTHER?
>> >> >
>> >> > What would the semantics even be? Any block device that doesn't pick up
>> >> > a different category may pick up IF_OTHER backends?
>> >> >
>> >> > It certainly feels like a strange interface to ask for "other" disk and
>> >> > then getting as surprise what this other thing might be. It's
>> >> > essentially the same as having an explicit '-device other', and I
>> >> > suppose most people would find that strange.
>> >> >
>> >> > > If no, I guess we get to abuse IF_PFLASH some more.
>> >> > >
>> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
>> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
>> >> > > be worth the trouble, though.
>> >> > >
>> >> > > Thoughts?
>> >> >
>> >> > If the existing types aren't good enough (I don't have an opinion on
>> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
>> >> > new one, not just "other".
>> >> 
>> >> I think the common thread is "this isn't what anybody actually thinks
>> >> of as being a 'disk', but we would like to back it with a block device
>> >> anyway". That can cover a fair range of possibilities...
>> >
>> > Given that, do we even want/have to use -drive for this ?    We can use
>> > -blockdev for the backend and reference that from any -device we want
>> > to create, and leave -drive out of the picture entirely
>> 
>> -drive is our only means to configure onboard devices.
>> 
>> We've talked about better means a few times, but no conclusions.  I can
>> dig up pointers, if you're interested.
>
> For onboard pflash with x86, we've just got properties against the
> machine that we can point to a blockdev.

True, but the vast majority of onboard block devices doesn't come with
such properties.  Please see

Subject: On configuring onboard block devices with -blockdev (was: [PATCH v4 6/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards)
Date: Mon, 15 Nov 2021 16:28:33 +0100
Message-ID: <875ystigke.fsf_-_@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg03173.html

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Daniel P. Berrangé]

On Thu, Aug 04, 2022 at 05:30:40PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Aug 04, 2022 at 04:56:15PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
> >> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> >> >> >
> >> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> >> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> >> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> >> >> > > other interface types, too.
> >> >> > >
> >> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> >> >> > > EEPROM device.
> >> >> > >
> >> >> > > Do we want IF_OTHER?
> >> >> >
> >> >> > What would the semantics even be? Any block device that doesn't pick up
> >> >> > a different category may pick up IF_OTHER backends?
> >> >> >
> >> >> > It certainly feels like a strange interface to ask for "other" disk and
> >> >> > then getting as surprise what this other thing might be. It's
> >> >> > essentially the same as having an explicit '-device other', and I
> >> >> > suppose most people would find that strange.
> >> >> >
> >> >> > > If no, I guess we get to abuse IF_PFLASH some more.
> >> >> > >
> >> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> >> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> >> >> > > be worth the trouble, though.
> >> >> > >
> >> >> > > Thoughts?
> >> >> >
> >> >> > If the existing types aren't good enough (I don't have an opinion on
> >> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> >> >> > new one, not just "other".
> >> >> 
> >> >> I think the common thread is "this isn't what anybody actually thinks
> >> >> of as being a 'disk', but we would like to back it with a block device
> >> >> anyway". That can cover a fair range of possibilities...
> >> >
> >> > Given that, do we even want/have to use -drive for this ?    We can use
> >> > -blockdev for the backend and reference that from any -device we want
> >> > to create, and leave -drive out of the picture entirely
> >> 
> >> -drive is our only means to configure onboard devices.
> >> 
> >> We've talked about better means a few times, but no conclusions.  I can
> >> dig up pointers, if you're interested.
> >
> > For onboard pflash with x86, we've just got properties against the
> > machine that we can point to a blockdev.
> 
> True, but the vast majority of onboard block devices doesn't come with
> such properties.  Please see
> 
> Subject: On configuring onboard block devices with -blockdev (was: [PATCH v4 6/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards)
> Date: Mon, 15 Nov 2021 16:28:33 +0100
> Message-ID: <875ystigke.fsf_-_@dusky.pond.sub.org>
> https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg03173.html

My take away from your mail there is that in the absence of better ideas
we should at least use machine properties for anything new we do so we
don't make the problem worse than it already is. It feels more useful
than inventing new IF_xxx possibilities for something we think is the
wrong approach already.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Aug 04, 2022 at 05:30:40PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Aug 04, 2022 at 04:56:15PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
>> >> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
>> >> >> >
>> >> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
>> >> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
>> >> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
>> >> >> > > other interface types, too.
>> >> >> > >
>> >> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
>> >> >> > > EEPROM device.
>> >> >> > >
>> >> >> > > Do we want IF_OTHER?
>> >> >> >
>> >> >> > What would the semantics even be? Any block device that doesn't pick up
>> >> >> > a different category may pick up IF_OTHER backends?
>> >> >> >
>> >> >> > It certainly feels like a strange interface to ask for "other" disk and
>> >> >> > then getting as surprise what this other thing might be. It's
>> >> >> > essentially the same as having an explicit '-device other', and I
>> >> >> > suppose most people would find that strange.
>> >> >> >
>> >> >> > > If no, I guess we get to abuse IF_PFLASH some more.
>> >> >> > >
>> >> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
>> >> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
>> >> >> > > be worth the trouble, though.
>> >> >> > >
>> >> >> > > Thoughts?
>> >> >> >
>> >> >> > If the existing types aren't good enough (I don't have an opinion on
>> >> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
>> >> >> > new one, not just "other".
>> >> >> 
>> >> >> I think the common thread is "this isn't what anybody actually thinks
>> >> >> of as being a 'disk', but we would like to back it with a block device
>> >> >> anyway". That can cover a fair range of possibilities...
>> >> >
>> >> > Given that, do we even want/have to use -drive for this ?    We can use
>> >> > -blockdev for the backend and reference that from any -device we want
>> >> > to create, and leave -drive out of the picture entirely
>> >> 
>> >> -drive is our only means to configure onboard devices.
>> >> 
>> >> We've talked about better means a few times, but no conclusions.  I can
>> >> dig up pointers, if you're interested.
>> >
>> > For onboard pflash with x86, we've just got properties against the
>> > machine that we can point to a blockdev.
>> 
>> True, but the vast majority of onboard block devices doesn't come with
>> such properties.  Please see
>> 
>> Subject: On configuring onboard block devices with -blockdev (was: [PATCH v4 6/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards)
>> Date: Mon, 15 Nov 2021 16:28:33 +0100
>> Message-ID: <875ystigke.fsf_-_@dusky.pond.sub.org>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg03173.html
>
> My take away from your mail there is that in the absence of better ideas
> we should at least use machine properties for anything new we do so we
> don't make the problem worse than it already is. It feels more useful
> than inventing new IF_xxx possibilities for something we think is the
> wrong approach already.

The difficulty of providing machine properties for existing devices and
the lack of adoption even for new devices make me doubt they are a
viable path forward.  In the message I referenced above, I wrote:

    If "replace them all by machine properties" is the way forward, we
    need to get going.  At the current rate of one file a year (measured
    charitably), we'll be done around 2090, provided we don't add more
    (we've added quite a few since I did the first replacement).

I figure this has slipped to the 22nd century by now.

Yes, more uses of -drive are steps backward.  But they are trivially
easy, and also drops in the bucket.  Machine properties are more
difficult, and whether they actually take us forward seems doubtful.

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Kevin Wolf]

Am 08.08.2022 um 08:26 hat Markus Armbruster geschrieben:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Aug 04, 2022 at 05:30:40PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Thu, Aug 04, 2022 at 04:56:15PM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >> 
> >> >> > On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
> >> >> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> >> >> >> >
> >> >> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> >> >> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> >> >> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> >> >> >> > > other interface types, too.
> >> >> >> > >
> >> >> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> >> >> >> > > EEPROM device.
> >> >> >> > >
> >> >> >> > > Do we want IF_OTHER?
> >> >> >> >
> >> >> >> > What would the semantics even be? Any block device that doesn't pick up
> >> >> >> > a different category may pick up IF_OTHER backends?
> >> >> >> >
> >> >> >> > It certainly feels like a strange interface to ask for "other" disk and
> >> >> >> > then getting as surprise what this other thing might be. It's
> >> >> >> > essentially the same as having an explicit '-device other', and I
> >> >> >> > suppose most people would find that strange.
> >> >> >> >
> >> >> >> > > If no, I guess we get to abuse IF_PFLASH some more.
> >> >> >> > >
> >> >> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> >> >> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> >> >> >> > > be worth the trouble, though.
> >> >> >> > >
> >> >> >> > > Thoughts?
> >> >> >> >
> >> >> >> > If the existing types aren't good enough (I don't have an opinion on
> >> >> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> >> >> >> > new one, not just "other".
> >> >> >> 
> >> >> >> I think the common thread is "this isn't what anybody actually thinks
> >> >> >> of as being a 'disk', but we would like to back it with a block device
> >> >> >> anyway". That can cover a fair range of possibilities...
> >> >> >
> >> >> > Given that, do we even want/have to use -drive for this ?    We can use
> >> >> > -blockdev for the backend and reference that from any -device we want
> >> >> > to create, and leave -drive out of the picture entirely
> >> >> 
> >> >> -drive is our only means to configure onboard devices.
> >> >> 
> >> >> We've talked about better means a few times, but no conclusions.  I can
> >> >> dig up pointers, if you're interested.
> >> >
> >> > For onboard pflash with x86, we've just got properties against the
> >> > machine that we can point to a blockdev.
> >> 
> >> True, but the vast majority of onboard block devices doesn't come with
> >> such properties.  Please see
> >> 
> >> Subject: On configuring onboard block devices with -blockdev (was: [PATCH v4 6/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards)
> >> Date: Mon, 15 Nov 2021 16:28:33 +0100
> >> Message-ID: <875ystigke.fsf_-_@dusky.pond.sub.org>
> >> https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg03173.html
> >
> > My take away from your mail there is that in the absence of better ideas
> > we should at least use machine properties for anything new we do so we
> > don't make the problem worse than it already is. It feels more useful
> > than inventing new IF_xxx possibilities for something we think is the
> > wrong approach already.
> 
> The difficulty of providing machine properties for existing devices and
> the lack of adoption even for new devices make me doubt they are a
> viable path forward.  In the message I referenced above, I wrote:
> 
>     If "replace them all by machine properties" is the way forward, we
>     need to get going.  At the current rate of one file a year (measured
>     charitably), we'll be done around 2090, provided we don't add more
>     (we've added quite a few since I did the first replacement).
> 
> I figure this has slipped to the 22nd century by now.
> 
> Yes, more uses of -drive are steps backward.  But they are trivially
> easy, and also drops in the bucket.  Machine properties are more
> difficult, and whether they actually take us forward seems doubtful.

Machine properties are also not what we really want, but just a
workaround. How about implementing the real thing, providing qdev
properties for built-in devices?

Looking at a few random users of drive_get(), they look like this:

    DriveInfo *dinfo = drive_get(...);
    dev = qdev_new("driver-type");
    qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
    qdev_realize_and_unref(...);

What if we assigned a name to every built-in device and had a
qdev_new_builtin(type, id) that applies qdev properties given on the
command line to the device? Could be either with plain old '-device'
(we're already doing a similar thing with default devices where adding
-device for the existing device removes the default device) or with a
new command line option.

The qdev_prop_set_drive() would then become conditional and would only
be done if the property wasn't already set in qdev_new_builtin(). Or
maybe a new function that checks for conflicting -drive and -device.
Though that part is just implementation details.

Kevin

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Markus Armbruster]

Adding Paolo and Eduardo since we're wandering into QOM-land.

Kevin Wolf <kwolf@redhat.com> writes:

> Am 08.08.2022 um 08:26 hat Markus Armbruster geschrieben:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Aug 04, 2022 at 05:30:40PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > On Thu, Aug 04, 2022 at 04:56:15PM +0200, Markus Armbruster wrote:
>> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> >> 
>> >> >> > On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
>> >> >> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
>> >> >> >> >
>> >> >> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
>> >> >> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
>> >> >> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
>> >> >> >> > > other interface types, too.
>> >> >> >> > >
>> >> >> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
>> >> >> >> > > EEPROM device.
>> >> >> >> > >
>> >> >> >> > > Do we want IF_OTHER?
>> >> >> >> >
>> >> >> >> > What would the semantics even be? Any block device that doesn't pick up
>> >> >> >> > a different category may pick up IF_OTHER backends?
>> >> >> >> >
>> >> >> >> > It certainly feels like a strange interface to ask for "other" disk and
>> >> >> >> > then getting as surprise what this other thing might be. It's
>> >> >> >> > essentially the same as having an explicit '-device other', and I
>> >> >> >> > suppose most people would find that strange.
>> >> >> >> >
>> >> >> >> > > If no, I guess we get to abuse IF_PFLASH some more.
>> >> >> >> > >
>> >> >> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
>> >> >> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
>> >> >> >> > > be worth the trouble, though.
>> >> >> >> > >
>> >> >> >> > > Thoughts?
>> >> >> >> >
>> >> >> >> > If the existing types aren't good enough (I don't have an opinion on
>> >> >> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
>> >> >> >> > new one, not just "other".
>> >> >> >> 
>> >> >> >> I think the common thread is "this isn't what anybody actually thinks
>> >> >> >> of as being a 'disk', but we would like to back it with a block device
>> >> >> >> anyway". That can cover a fair range of possibilities...
>> >> >> >
>> >> >> > Given that, do we even want/have to use -drive for this ?    We can use
>> >> >> > -blockdev for the backend and reference that from any -device we want
>> >> >> > to create, and leave -drive out of the picture entirely
>> >> >> 
>> >> >> -drive is our only means to configure onboard devices.
>> >> >> 
>> >> >> We've talked about better means a few times, but no conclusions.  I can
>> >> >> dig up pointers, if you're interested.
>> >> >
>> >> > For onboard pflash with x86, we've just got properties against the
>> >> > machine that we can point to a blockdev.
>> >> 
>> >> True, but the vast majority of onboard block devices doesn't come with
>> >> such properties.  Please see
>> >> 
>> >> Subject: On configuring onboard block devices with -blockdev (was: [PATCH v4 6/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards)
>> >> Date: Mon, 15 Nov 2021 16:28:33 +0100
>> >> Message-ID: <875ystigke.fsf_-_@dusky.pond.sub.org>
>> >> https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg03173.html
>> >
>> > My take away from your mail there is that in the absence of better ideas
>> > we should at least use machine properties for anything new we do so we
>> > don't make the problem worse than it already is. It feels more useful
>> > than inventing new IF_xxx possibilities for something we think is the
>> > wrong approach already.
>> 
>> The difficulty of providing machine properties for existing devices and
>> the lack of adoption even for new devices make me doubt they are a
>> viable path forward.  In the message I referenced above, I wrote:
>> 
>>     If "replace them all by machine properties" is the way forward, we
>>     need to get going.  At the current rate of one file a year (measured
>>     charitably), we'll be done around 2090, provided we don't add more
>>     (we've added quite a few since I did the first replacement).
>> 
>> I figure this has slipped to the 22nd century by now.
>> 
>> Yes, more uses of -drive are steps backward.  But they are trivially
>> easy, and also drops in the bucket.  Machine properties are more
>> difficult, and whether they actually take us forward seems doubtful.
>
> Machine properties are also not what we really want, but just a
> workaround. How about implementing the real thing, providing qdev
> properties for built-in devices?
>
> Looking at a few random users of drive_get(), they look like this:
>
>     DriveInfo *dinfo = drive_get(...);
>     dev = qdev_new("driver-type");
>     qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>     qdev_realize_and_unref(...);
>
> What if we assigned a name to every built-in device

Before we continue: I like the idea of giving onboard devices
user-friendly, stable names.  "serial0" beats
"/machine/unattached/device[13]" hands down.  Even more so where the
"device[13]" part depends on other options (e.g. with "-vga none" it's
"device[12]", both for i440FX).

The question is what these names could be.  They can't be qdev IDs,
because we blew our chance to reserve names.

Could we solve this in QOM?

>                                                     and had a
> qdev_new_builtin(type, id) that applies qdev properties given on the
> command line to the device? Could be either with plain old '-device'
> (we're already doing a similar thing with default devices where adding
> -device for the existing device removes the default device) or with a
> new command line option.
>
> The qdev_prop_set_drive() would then become conditional and would only
> be done if the property wasn't already set in qdev_new_builtin(). Or
> maybe a new function that checks for conflicting -drive and -device.
> Though that part is just implementation details.

The idea sounds vaguely familiar.  Whether it's because we discussed
similar ones on the list, or because I mulled over similar ones in my
head I can't say for sure.

Overloading -device so it can also configure existing devices feels
iffy.

We already have means to set properties for (onboard) devices to pick
up: -global.  But it's per device *type*, and therefore falls apart as
soon as you have more than one of the type.  We need something that
affects a specific (onboard) device, and for that we need a way to
address one.  The QOM paths we have don't cut it, as I illustrated
above.

[PATCH v5 6/8] hw/arm: npcm8xx_boards: EEPROMs can take bus as parameter

[Hao Wu]

We allow at24c_eeprom_init to take a I2CBus* as parameter. This allows
us to attach an EEPROM device behind an I2C mux which is not
possible with the old method.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 hw/arm/npcm7xx_boards.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 6bc6f5d2fe..b083b0c572 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -140,10 +140,9 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num)
     return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus"));
 }
 
-static void at24c_eeprom_init(NPCM7xxState *soc, int bus, uint8_t addr,
+static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
                               uint32_t rsize)
 {
-    I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
     I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
     DeviceState *dev = DEVICE(i2c_dev);
 
@@ -253,8 +252,8 @@ static void quanta_gsj_i2c_init(NPCM7xxState *soc)
     i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "tmp105", 0x5c);
     i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), "tmp105", 0x5c);
 
-    at24c_eeprom_init(soc, 9, 0x55, 8192);
-    at24c_eeprom_init(soc, 10, 0x55, 8192);
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 9), 9, 0x55, 8192);
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 10), 10, 0x55, 8192);
 
     /*
      * i2c-11:
@@ -360,7 +359,8 @@ static void kudo_bmc_i2c_init(NPCM7xxState *soc)
 
     i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), TYPE_PCA9548, 0x77);
 
-    at24c_eeprom_init(soc, 4, 0x50, 8192); /* mbfru */
+    /* mbfru */
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 4), 4, 0x50, 8192);
 
     i2c_mux = i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 13),
                                       TYPE_PCA9548, 0x77);
@@ -371,7 +371,8 @@ static void kudo_bmc_i2c_init(NPCM7xxState *soc)
     i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 4), "tmp105", 0x48);
     i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 5), "tmp105", 0x49);
 
-    at24c_eeprom_init(soc, 14, 0x55, 8192); /* bmcfru */
+    /* bmcfru */
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 14), 14, 0x55, 8192);
 
     /* TODO: Add remaining i2c devices. */
 }
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v5 7/8] hw/arm: Set drive property for at24c eeprom

[Hao Wu]

This patch allows the user to attach an external drive as a property
for an onboard at24c eeprom device. It uses an unit number to
distinguish different devices.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 hw/arm/npcm7xx_boards.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index b083b0c572..b8337871ba 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -141,11 +141,16 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num)
 }
 
 static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
-                              uint32_t rsize)
+                              uint32_t rsize, int unit)
 {
     I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
     DeviceState *dev = DEVICE(i2c_dev);
+    DriveInfo *dinfo;
 
+    dinfo = drive_get(IF_OTHER, bus, unit);
+    if (dinfo) {
+        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
+    }
     qdev_prop_set_uint32(dev, "rom-size", rsize);
     i2c_slave_realize_and_unref(i2c_dev, i2c_bus, &error_abort);
 }
@@ -252,8 +257,8 @@ static void quanta_gsj_i2c_init(NPCM7xxState *soc)
     i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "tmp105", 0x5c);
     i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), "tmp105", 0x5c);
 
-    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 9), 9, 0x55, 8192);
-    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 10), 10, 0x55, 8192);
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 9), 9, 0x55, 8192, 0);
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 10), 10, 0x55, 8192, 1);
 
     /*
      * i2c-11:
@@ -360,7 +365,7 @@ static void kudo_bmc_i2c_init(NPCM7xxState *soc)
     i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), TYPE_PCA9548, 0x77);
 
     /* mbfru */
-    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 4), 4, 0x50, 8192);
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 4), 4, 0x50, 8192, 0);
 
     i2c_mux = i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 13),
                                       TYPE_PCA9548, 0x77);
@@ -372,7 +377,7 @@ static void kudo_bmc_i2c_init(NPCM7xxState *soc)
     i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 5), "tmp105", 0x49);
 
     /* bmcfru */
-    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 14), 14, 0x55, 8192);
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 14), 14, 0x55, 8192, 1);
 
     /* TODO: Add remaining i2c devices. */
 }
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v5 7/8] hw/arm: Set drive property for at24c eeprom

[Markus Armbruster]

Hao Wu <wuhaotsh@google.com> writes:

> This patch allows the user to attach an external drive as a property
> for an onboard at24c eeprom device.

What's the contents of the EEPROM before the patch?

I guess the patch lets users replace that contents.  Why would a user
want to do that?

>                                     It uses an unit number to
> distinguish different devices.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>

[PATCH v5 8/8] hw/arm: quanta-gbs-bmc add i2c devices

[Hao Wu]

From: Patrick Venture <venture@google.com>

Adds supported i2c devices to the quanta-gbc-bmc board.

Signed-off-by: Patrick Venture <venture@google.com>
Reviewed-by: Hao Wu <wuhaotsh@google.com>
---
 hw/arm/npcm7xx_boards.c | 82 ++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index b8337871ba..4bae5589f0 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -290,10 +290,12 @@ static void quanta_gsj_fan_init(NPCM7xxMachine *machine, NPCM7xxState *soc)
 
 static void quanta_gbs_i2c_init(NPCM7xxState *soc)
 {
+    I2CSlave *i2c_mux;
+
+    /* i2c-0: */
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 0), TYPE_PCA9546, 0x71);
+
     /*
-     * i2c-0:
-     *     pca9546@71
-     *
      * i2c-1:
      *     pca9535@24
      *     pca9535@20
@@ -302,46 +304,60 @@ static void quanta_gbs_i2c_init(NPCM7xxState *soc)
      *     pca9535@23
      *     pca9535@25
      *     pca9535@26
-     *
-     * i2c-2:
-     *     sbtsi@4c
-     *
-     * i2c-5:
-     *     atmel,24c64@50 mb_fru
-     *     pca9546@71
-     *         - channel 0: max31725@54
-     *         - channel 1: max31725@55
-     *         - channel 2: max31725@5d
-     *                      atmel,24c64@51 fan_fru
-     *         - channel 3: atmel,24c64@52 hsbp_fru
-     *
+     */
+
+    /* i2c-2: sbtsi@4c */
+
+    /* i2c-5: */
+    /* mb_fru */
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 5), 5, 0x50, 8192, 0);
+    i2c_mux = i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 5),
+                                      TYPE_PCA9546, 0x71);
+    /* max31725 is tmp105 compatible. */
+    i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 0), "tmp105", 0x54);
+    i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 1), "tmp105", 0x55);
+    i2c_slave_create_simple(pca954x_i2c_get_bus(i2c_mux, 2), "tmp105", 0x5d);
+    /* fan_fru */
+    at24c_eeprom_init(pca954x_i2c_get_bus(i2c_mux, 2), 5, 0x51, 8192, 1);
+    /* hsbp_fru */
+    at24c_eeprom_init(pca954x_i2c_get_bus(i2c_mux, 3), 5, 0x52, 8192, 2);
+
+    /*
      * i2c-6:
      *     pca9545@73
      *
      * i2c-7:
      *     pca9545@72
-     *
-     * i2c-8:
-     *     adi,adm1272@10
-     *
-     * i2c-9:
-     *     pca9546@71
-     *         - channel 0: isil,isl68137@60
-     *         - channel 1: isil,isl68137@61
-     *         - channel 2: isil,isl68137@63
-     *         - channel 3: isil,isl68137@45
-     *
+     */
+
+    /* i2c-8: */
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 8), "adm1272", 0x10);
+
+    /* i2c-9: */
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 9), TYPE_PCA9546, 0x71);
+    /*
+     * - channel 0: isil,isl68137@60
+     * - channel 1: isil,isl68137@61
+     * - channel 2: isil,isl68137@63
+     * - channel 3: isil,isl68137@45
+     */
+
+    /*
      * i2c-10:
      *     pca9545@71
      *
      * i2c-11:
      *     pca9545@76
-     *
-     * i2c-12:
-     *     maxim,max34451@4e
-     *     isil,isl68137@5d
-     *     isil,isl68137@5e
-     *
+     */
+
+    /* i2c-12: */
+    i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 12), "max34451", 0x4e);
+    /*
+     * isil,isl68137@5d
+     * isil,isl68137@5e
+     */
+
+    /*
      * i2c-14:
      *     pca9545@70
      */
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v5 0/8] Misc NPCM7XX patches

[Peter Maydell]

On Thu, 14 Jul 2022 at 19:28, Hao Wu <wuhaotsh@google.com> wrote:
>
> [NOTE: I'm reviving a bunch of patches that was in the process of
> upstreaming a while ago but paused.]
>
> This patch set contains a few bug fixes and I2C devices for some
> NPCM7XX boards.
>
> Patch 1~2 fix a problem that causes the SMBus module to behave
> incorrectly when it's in FIFO mode and trying to receive more than
> 16 bytes at a time.
>
> Patch 3 fixes a error in a register for ADC module.
>
> Patch 4 makes the ADC input to be R/W instead of write only. It allows
> a test system to read these via QMP and has no negative effect.
>
> Patch 5 adds a new blockdev IF type IF_OTHER.
>
> Patch 6 allows at24c_eeprom_init to take a bus as parameter so it can
> be used by more use cases (e.g. behind an I2C mux.)
>
> Patch 7 allows at24c_eeprom_init to take a drive as property, similar
> to sdhci_attach_device().
>
> Patch 8 uses the function defined in patch 5 to add the EEPROM and other
> I2C devices for Quanta GBS board.

I've taken patches 3 and 4 into target-arm.next as they're
already reviewed and standalone bugfixes. I had a comment on
patch 2. I'll leave the IF_OTHER patch (and the patches that
make use of it) to the blockdev folks to review.

thanks
-- PMM