[PATCH v2 0/7] Misc NPCM7XX patches

[PATCH v2 0/7] Misc NPCM7XX patches

[Hao Wu]

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 modifies at24c_eeprom_init in NPCM7xx boards so that it can fit
more use cases.

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

Patch 7 adds an ID for SMBus devices in NPCM7xx boards. This allows us to
add device to these buses using "-device" parameter.

-- 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 (6):
  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
  hw/nvram: Update at24c EEPROM init function in NPCM7xx boards
  hw/arm: Add ID for NPCM7XX SMBus

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

 hw/adc/npcm7xx_adc.c           |  4 +-
 hw/arm/npcm7xx.c               |  1 +
 hw/arm/npcm7xx_boards.c        | 97 +++++++++++++++++++++-------------
 hw/i2c/npcm7xx_smbus.c         |  8 +--
 tests/qtest/npcm7xx_adc-test.c |  2 +-
 5 files changed, 67 insertions(+), 45 deletions(-)

-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH v2 1/7] 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>
---
 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.33.0.1079.g6e70778dc9-goog

Re: [PATCH v2 1/7] hw/i2c: Clear ACK bit in NPCM7xx SMBus module

[Peter Maydell]

On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>
> 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>

thanks
-- PMM

[PATCH v2 2/7] 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>
---
 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.33.0.1079.g6e70778dc9-goog

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

[Corey Minyard]

On Thu, Oct 21, 2021 at 11:39:51AM -0700, Hao Wu 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>

This looks ok.  I assume you can take this in with the rest of the
patches.

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.33.0.1079.g6e70778dc9-goog
> 
> 

[PATCH v2 3/7] 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>
---
 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 5ce8ce13b3..aaf127dd42 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.33.0.1079.g6e70778dc9-goog

Re: [PATCH v2 3/7] hw/adc: Fix CONV bit in NPCM7XX ADC CON register

[Peter Maydell]

On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>
> 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>

thanks
-- PMM

[PATCH v2 4/7] 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 read only for that to work.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Titus Rwantare <titusr@google.com>
---
 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.33.0.1079.g6e70778dc9-goog

Re: [PATCH v2 4/7] hw/adc: Make adci[*] R/W in NPCM7XX ADC

[Peter Maydell]

On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>
> 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 read only for that to work.

"instead of write only", I think ?

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 5/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards

[Hao Wu]

We made 3 changes to the at24c_eeprom_init function in
npcm7xx_boards.c:

1. We allow the function 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.

2. We make at24c EEPROMs are backed by drives so that we can
   specify the content of the EEPROMs.

3. Instead of using i2c address as unit number, This patch assigns
   unique unit numbers for each eeproms in each board. This avoids
   conflict in providing multiple eeprom contents with the same address.
   In the old method if we specify two drives with the same unit number,
   the following error will occur: `Device with id 'none85' exists`.

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 a656169f61..cdb52b9922 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -107,13 +107,18 @@ 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,
-                              uint32_t rsize)
+static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
+                              uint32_t rsize, int unit_number)
 {
-    I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
     I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
     DeviceState *dev = DEVICE(i2c_dev);
+    BlockInterfaceType type = IF_NONE;
+    DriveInfo *dinfo;
 
+    dinfo = drive_get(type, bus, unit_number);
+    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);
 }
@@ -220,8 +225,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, 0);
+    at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 10), 10, 0x55, 8192, 1);
 
     /*
      * i2c-11:
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH v2 5/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards

[Peter Maydell]

On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>
> We made 3 changes to the at24c_eeprom_init function in
> npcm7xx_boards.c:
>
> 1. We allow the function 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.
>
> 2. We make at24c EEPROMs are backed by drives so that we can
>    specify the content of the EEPROMs.
>
> 3. Instead of using i2c address as unit number, This patch assigns
>    unique unit numbers for each eeproms in each board. This avoids
>    conflict in providing multiple eeprom contents with the same address.
>    In the old method if we specify two drives with the same unit number,
>    the following error will occur: `Device with id 'none85' exists`.
>
> 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 a656169f61..cdb52b9922 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -107,13 +107,18 @@ 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,
> -                              uint32_t rsize)
> +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
> +                              uint32_t rsize, int unit_number)
>  {
> -    I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
>      I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
>      DeviceState *dev = DEVICE(i2c_dev);
> +    BlockInterfaceType type = IF_NONE;

Why make this a variable? We only use it in one place...

-- PMM

Re: [PATCH v2 5/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards

[Hao Wu]

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

On Mon, Nov 1, 2021 at 10:41 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
> >
> > We made 3 changes to the at24c_eeprom_init function in
> > npcm7xx_boards.c:
> >
> > 1. We allow the function 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.
> >
> > 2. We make at24c EEPROMs are backed by drives so that we can
> >    specify the content of the EEPROMs.
> >
> > 3. Instead of using i2c address as unit number, This patch assigns
> >    unique unit numbers for each eeproms in each board. This avoids
> >    conflict in providing multiple eeprom contents with the same address.
> >    In the old method if we specify two drives with the same unit number,
> >    the following error will occur: `Device with id 'none85' exists`.
> >
> > 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 a656169f61..cdb52b9922 100644
> > --- a/hw/arm/npcm7xx_boards.c
> > +++ b/hw/arm/npcm7xx_boards.c
> > @@ -107,13 +107,18 @@ 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,
> > -                              uint32_t rsize)
> > +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
> > +                              uint32_t rsize, int unit_number)
> >  {
> > -    I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
> >      I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
> >      DeviceState *dev = DEVICE(i2c_dev);
> > +    BlockInterfaceType type = IF_NONE;
>
> Why make this a variable? We only use it in one place...
>
You're right, we can actually inline it.

>
> -- PMM
>

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

Re: [PATCH v2 5/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards

[Thomas Huth]

On 01/11/2021 18.47, Hao Wu wrote:
> 
> 
> On Mon, Nov 1, 2021 at 10:41 AM Peter Maydell <peter.maydell@linaro.org 
> <mailto:peter.maydell@linaro.org>> wrote:
> 
>     On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com
>     <mailto:wuhaotsh@google.com>> wrote:
>      >
>      > We made 3 changes to the at24c_eeprom_init function in
>      > npcm7xx_boards.c:
>      >
>      > 1. We allow the function 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.
>      >
>      > 2. We make at24c EEPROMs are backed by drives so that we can
>      >    specify the content of the EEPROMs.
>      >
>      > 3. Instead of using i2c address as unit number, This patch assigns
>      >    unique unit numbers for each eeproms in each board. This avoids
>      >    conflict in providing multiple eeprom contents with the same address.
>      >    In the old method if we specify two drives with the same unit number,
>      >    the following error will occur: `Device with id 'none85' exists`.
>      >
>      > Signed-off-by: Hao Wu <wuhaotsh@google.com <mailto: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 a656169f61..cdb52b9922 100644
>      > --- a/hw/arm/npcm7xx_boards.c
>      > +++ b/hw/arm/npcm7xx_boards.c
>      > @@ -107,13 +107,18 @@ 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,
>      > -                              uint32_t rsize)
>      > +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
>      > +                              uint32_t rsize, int unit_number)
>      >  {
>      > -    I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
>      >      I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
>      >      DeviceState *dev = DEVICE(i2c_dev);
>      > +    BlockInterfaceType type = IF_NONE;
> 
>     Why make this a variable? We only use it in one place...
> 
> You're right, we can actually inline it.

Actually, please do *not* use IF_NONE for such stuff. See the discussion 
here for details:

  https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00970.html

  Thomas

Re: [PATCH v2 5/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards

[Hao Wu]

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

Thanks for the comment. I'll upload a new version using IF_OTHER as
discussed in that thread.

I don't know if adding the assertion for type != IF_NONE is a good idea now
so I didn't add that.
If you think that's good I can add it as well.

On Wed, Nov 3, 2021 at 2:13 AM Thomas Huth <thuth@redhat.com> wrote:

> On 01/11/2021 18.47, Hao Wu wrote:
> >
> >
> > On Mon, Nov 1, 2021 at 10:41 AM Peter Maydell <peter.maydell@linaro.org
> > <mailto:peter.maydell@linaro.org>> wrote:
> >
> >     On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com
> >     <mailto:wuhaotsh@google.com>> wrote:
> >      >
> >      > We made 3 changes to the at24c_eeprom_init function in
> >      > npcm7xx_boards.c:
> >      >
> >      > 1. We allow the function 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.
> >      >
> >      > 2. We make at24c EEPROMs are backed by drives so that we can
> >      >    specify the content of the EEPROMs.
> >      >
> >      > 3. Instead of using i2c address as unit number, This patch assigns
> >      >    unique unit numbers for each eeproms in each board. This avoids
> >      >    conflict in providing multiple eeprom contents with the same
> address.
> >      >    In the old method if we specify two drives with the same unit
> number,
> >      >    the following error will occur: `Device with id 'none85'
> exists`.
> >      >
> >      > Signed-off-by: Hao Wu <wuhaotsh@google.com <mailto:
> 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 a656169f61..cdb52b9922 100644
> >      > --- a/hw/arm/npcm7xx_boards.c
> >      > +++ b/hw/arm/npcm7xx_boards.c
> >      > @@ -107,13 +107,18 @@ 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,
> >      > -                              uint32_t rsize)
> >      > +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t
> addr,
> >      > +                              uint32_t rsize, int unit_number)
> >      >  {
> >      > -    I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
> >      >      I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
> >      >      DeviceState *dev = DEVICE(i2c_dev);
> >      > +    BlockInterfaceType type = IF_NONE;
> >
> >     Why make this a variable? We only use it in one place...
> >
> > You're right, we can actually inline it.
>
> Actually, please do *not* use IF_NONE for such stuff. See the discussion
> here for details:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00970.html
>
>   Thomas
>
>

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

Re: [PATCH v2 5/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards

[Peter Maydell]

On Wed, 3 Nov 2021 at 21:52, Hao Wu <wuhaotsh@google.com> wrote:
>
> Thanks for the comment. I'll upload a new version using IF_OTHER as discussed in that thread.
>
> I don't know if adding the assertion for type != IF_NONE is a good idea now so I didn't add that.
> If you think that's good I can add it as well.

We would have to fix the existing use of IF_NONE in the tree
before we could add an assert. (Awkward because it will
break commandlines for that existing use.)

-- PMM

[PATCH v2 6/7] 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 cdb52b9922..b09919df81 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -258,10 +258,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
@@ -270,46 +272,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.33.0.1079.g6e70778dc9-goog

[PATCH v2 7/7] hw/arm: Add ID for NPCM7XX SMBus

[Hao Wu]

The ID can be used to indicate SMBus modules when adding
dynamic devices to them.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 hw/arm/npcm7xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index 2ab0080e0b..72953d65ef 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj)
     for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
         object_initialize_child(obj, "smbus[*]", &s->smbus[i],
                                 TYPE_NPCM7XX_SMBUS);
+        DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i);
     }
 
     object_initialize_child(obj, "ehci", &s->ehci, TYPE_NPCM7XX_EHCI);
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH v2 7/7] hw/arm: Add ID for NPCM7XX SMBus

[Peter Maydell]

On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>
> The ID can be used to indicate SMBus modules when adding
> dynamic devices to them.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  hw/arm/npcm7xx.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> index 2ab0080e0b..72953d65ef 100644
> --- a/hw/arm/npcm7xx.c
> +++ b/hw/arm/npcm7xx.c
> @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj)
>      for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
>          object_initialize_child(obj, "smbus[*]", &s->smbus[i],
>                                  TYPE_NPCM7XX_SMBUS);
> +        DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i);
>      }

This one looks weird to me -- I'm pretty sure we shouldn't be messing
about with the DeviceState id string like that. It's supposed to be
internal to the QOM/qdev code.

-- PMM

Re: [PATCH v2 7/7] hw/arm: Add ID for NPCM7XX SMBus

[Hao Wu]

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

I was trying to allow attaching a device using "-device xxx,bus=smbus[0]"
Maybe there's a better way to allow that?

I guess I can drop this one from the patch set.

On Mon, Nov 1, 2021 at 10:33 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
> >
> > The ID can be used to indicate SMBus modules when adding
> > dynamic devices to them.
> >
> > Signed-off-by: Hao Wu <wuhaotsh@google.com>
> > ---
> >  hw/arm/npcm7xx.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> > index 2ab0080e0b..72953d65ef 100644
> > --- a/hw/arm/npcm7xx.c
> > +++ b/hw/arm/npcm7xx.c
> > @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj)
> >      for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
> >          object_initialize_child(obj, "smbus[*]", &s->smbus[i],
> >                                  TYPE_NPCM7XX_SMBUS);
> > +        DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i);
> >      }
>
> This one looks weird to me -- I'm pretty sure we shouldn't be messing
> about with the DeviceState id string like that. It's supposed to be
> internal to the QOM/qdev code.
>
> -- PMM
>

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

Re: [PATCH v2 7/7] hw/arm: Add ID for NPCM7XX SMBus

[Philippe Mathieu-Daudé]

+Markus/Eduardo

On 11/1/21 18:33, Peter Maydell wrote:
> On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>>
>> The ID can be used to indicate SMBus modules when adding
>> dynamic devices to them.
>>
>> Signed-off-by: Hao Wu <wuhaotsh@google.com>
>> ---
>>  hw/arm/npcm7xx.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
>> index 2ab0080e0b..72953d65ef 100644
>> --- a/hw/arm/npcm7xx.c
>> +++ b/hw/arm/npcm7xx.c
>> @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj)
>>      for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
>>          object_initialize_child(obj, "smbus[*]", &s->smbus[i],
>>                                  TYPE_NPCM7XX_SMBUS);
>> +        DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i);
>>      }
> 
> This one looks weird to me -- I'm pretty sure we shouldn't be messing
> about with the DeviceState id string like that. It's supposed to be
> internal to the QOM/qdev code.

I agree with you, however:

$ git grep -F -- '->id = g_strdup' hw
hw/arm/virt.c:1512:    dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
hw/block/xen-block.c:761:    drive->id = g_strdup(id);
hw/block/xen-block.c:853:    iothread->id = g_strdup(id);
hw/core/machine-qmp-cmds.c:169:        m->id =
g_strdup(object_get_canonical_path_component(obj));
hw/mem/pc-dimm.c:244:        di->id = g_strdup(dev->id);
hw/pci-bridge/pci_expander_bridge.c:248:        bds->id =
g_strdup(dev_name);
hw/ppc/e500.c:1009:        dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
hw/s390x/s390-pci-bus.c:1003:            dev->id =
g_strdup_printf("auto_%02x:%02x.%01x",
hw/sh4/sh7750.c:819:    dev->id = g_strdup("sci");
hw/sh4/sh7750.c:836:    dev->id = g_strdup("scif");
hw/virtio/virtio-mem-pci.c:69:        vi->id = g_strdup(dev->id);
hw/virtio/virtio-pmem-pci.c:74:        vi->id = g_strdup(dev->id);

Re: [PATCH v2 7/7] hw/arm: Add ID for NPCM7XX SMBus

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> +Markus/Eduardo
>
> On 11/1/21 18:33, Peter Maydell wrote:
>> On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>>>
>>> The ID can be used to indicate SMBus modules when adding
>>> dynamic devices to them.
>>>
>>> Signed-off-by: Hao Wu <wuhaotsh@google.com>
>>> ---
>>>  hw/arm/npcm7xx.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
>>> index 2ab0080e0b..72953d65ef 100644
>>> --- a/hw/arm/npcm7xx.c
>>> +++ b/hw/arm/npcm7xx.c
>>> @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj)
>>>      for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
>>>          object_initialize_child(obj, "smbus[*]", &s->smbus[i],
>>>                                  TYPE_NPCM7XX_SMBUS);
>>> +        DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i);
>>>      }
>> 
>> This one looks weird to me -- I'm pretty sure we shouldn't be messing
>> about with the DeviceState id string like that. It's supposed to be
>> internal to the QOM/qdev code.

It's meant for the user.  Devices created by code shouldn't set it.  Not
least because any ID they pick could clash with the user's.

> I agree with you, however:
>
> $ git grep -F -- '->id = g_strdup' hw
> hw/arm/virt.c:1512:    dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> hw/block/xen-block.c:761:    drive->id = g_strdup(id);
> hw/block/xen-block.c:853:    iothread->id = g_strdup(id);
> hw/core/machine-qmp-cmds.c:169:        m->id = g_strdup(object_get_canonical_path_component(obj));
> hw/mem/pc-dimm.c:244:        di->id = g_strdup(dev->id);
> hw/pci-bridge/pci_expander_bridge.c:248:        bds->id = g_strdup(dev_name);
> hw/ppc/e500.c:1009:        dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> hw/s390x/s390-pci-bus.c:1003:            dev->id = g_strdup_printf("auto_%02x:%02x.%01x",
> hw/sh4/sh7750.c:819:    dev->id = g_strdup("sci");
> hw/sh4/sh7750.c:836:    dev->id = g_strdup("scif");
> hw/virtio/virtio-mem-pci.c:69:        vi->id = g_strdup(dev->id);
> hw/virtio/virtio-pmem-pci.c:74:        vi->id = g_strdup(dev->id);

This includes false positives, i.e. assignments to members of structs
other than DeviceState.  It also misses other ways to mess with
DeviceState member id.

Compiling with

    diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
    index 1bad07002d..b17ccd6065 100644
    --- a/include/hw/qdev-core.h
    +++ b/include/hw/qdev-core.h
    @@ -176,7 +176,7 @@ struct DeviceState {
         Object parent_obj;
         /*< public >*/

    -    char *id;
    +    char *const id;
         char *canonical_path;
         bool realized;
         bool pending_deleted_event;

ferrets out the actual assignments.  I got:

../hw/ppc/spapr_vio.c: In function ‘spapr_vio_busdev_realize’:
../hw/ppc/spapr_vio.c:505:22: error: assignment of read-only member ‘id’
  505 |         dev->qdev.id = id;
      |                      ^

This supplies a default ID to a vio-spapr-device.  Feels like a bad
idea.

../softmmu/qdev-monitor.c: In function ‘qdev_set_id’:
../softmmu/qdev-monitor.c:593:21: error: assignment of read-only member ‘id’
  593 |             dev->id = id;
      |                     ^

This assigns the user-supplied ID.

../hw/dma/xlnx-zdma.c: In function ‘zdma_realize’:
../hw/dma/xlnx-zdma.c:777:12: error: assignment of read-only location ‘*r’
  777 |         *r = (RegisterInfo) {
      |            ^

This *clobbers* the DeviceState embedded in *r, including its member id.
Suspicious.

../hw/pci-bridge/pci_expander_bridge.c: In function ‘pxb_dev_realize_common’:
../hw/pci-bridge/pci_expander_bridge.c:248:17: error: assignment of read-only member ‘id’
  248 |         bds->id = g_strdup(dev_name);
      |                 ^

This creates a pci-bridge device and gives it the same ID as the pxb
device being realized.  Doesn't this result in duplicate IDs?!?

../hw/arm/virt.c: In function ‘create_platform_bus’:
../hw/arm/virt.c:1512:13: error: assignment of read-only member ‘id’
 1512 |     dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
      |             ^

Helper function to create a platform-bus-device.  ID is set to
"platform-bus-device".  Feels like a bad idea.

../hw/ppc/e500.c: In function ‘ppce500_init’:
../hw/ppc/e500.c:1009:17: error: assignment of read-only member ‘id’
 1009 |         dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
      |                 ^

Likewise.

../hw/s390x/s390-pci-bus.c: In function ‘s390_pcihost_plug’:
../hw/s390x/s390-pci-bus.c:1003:21: error: assignment of read-only member ‘id’
 1003 |             dev->id = g_strdup_printf("auto_%02x:%02x.%01x",
      |                     ^

This supplies a default ID to a the device being plugged (I think).
Feels like a bad idea.

../hw/sh4/sh7750.c: In function ‘sh7750_init’:
../hw/sh4/sh7750.c:819:13: error: assignment of read-only member ‘id’
  819 |     dev->id = g_strdup("sci");
      |             ^
../hw/sh4/sh7750.c:836:13: error: assignment of read-only member ‘id’
  836 |     dev->id = g_strdup("scif");
      |             ^


These create sh-serial devices.  Their IDs are set to "sci" and "scif",
respectively.  Feels like a bad idea.