[PATCH 0/5] PMBus fixes and new functions

[PATCH 0/5] PMBus fixes and new functions

[Titus Rwantare]

This patch series contains fixes and improvements to PMBus support in QEMU.

The following has been added:
   - Support for block receive
   - Support for devices with fans
   - Support for the VCAP register for devices with onboard energy storage
   - A bitfield struct for the vout mode register, whose bits determine the formatting of several read commands in PMBus
Fixes:
   - String read now handles now logs an error when passed an empty string

This series is in preparation for some additional sensors that exercise
this functionality that will be incoming shortly.

Thanks

Titus Rwantare (5):
  hw/i2c: pmbus add support for block receive
  hw/i2c: pmbus: add vout mode bitfields
  hw/i2c: pmbus: add fan support
  hw/i2c: pmbus: block uninitialised string reads
  hw/i2c: pmbus: add VCAP register

 hw/i2c/pmbus_device.c         | 221 +++++++++++++++++++++++++++++++++-
 include/hw/i2c/pmbus_device.h |  17 +++
 2 files changed, 237 insertions(+), 1 deletion(-)

-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 1/5] hw/i2c: pmbus add support for block receive

[Titus Rwantare]

Reviewed-by: Hao Wu <wuhaotsh@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/i2c/pmbus_device.c         | 30 +++++++++++++++++++++++++++++-
 include/hw/i2c/pmbus_device.h |  7 +++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index c3d6046784..02647769cd 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
 void pmbus_send_string(PMBusDevice *pmdev, const char *data)
 {
     size_t len = strlen(data);
-    g_assert(len > 0);
     g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
     pmdev->out_buf[len + pmdev->out_buf_len] = len;
 
@@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
     pmdev->out_buf_len += len + 1;
 }
 
+uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len)
+{
+    /* dest may contain data from previous writes */
+    memset(dest, 0, len);
+
+    /* Exclude command code from return value */
+    pmdev->in_buf++;
+    pmdev->in_buf_len--;
+
+    /* The byte after the command code denotes the length */
+    uint8_t sent_len = pmdev->in_buf[0];
+
+    if (sent_len != pmdev->in_buf_len - 1) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: length mismatch. Expected %d bytes, got %d bytes\n",
+                      __func__, sent_len, pmdev->in_buf_len - 1);
+    }
+
+    /* exclude length byte */
+    pmdev->in_buf++;
+    pmdev->in_buf_len--;
+
+    if (pmdev->in_buf_len < len) {
+        len = pmdev->in_buf_len;
+    }
+    memcpy(dest, pmdev->in_buf, len);
+    return len;
+}
+
 
 static uint64_t pmbus_receive_uint(PMBusDevice *pmdev)
 {
diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index 93f5d57c9d..7dc00cc4d9 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t data);
  */
 void pmbus_send_string(PMBusDevice *state, const char *data);
 
+/**
+ * @brief Receive data sent with Block Write.
+ * @param dest - memory with enough capacity to receive the write
+ * @param len - the capacity of dest
+ */
+uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len);
+
 /**
  * @brief Receive data over PMBus
  * These methods help track how much data is being received over PMBus
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 2/5] hw/i2c: pmbus: add vout mode bitfields

[Titus Rwantare]

Reviewed-by: Hao Wu <wuhaotsh@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 include/hw/i2c/pmbus_device.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index 7dc00cc4d9..2e95164aa1 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -444,6 +444,14 @@ typedef struct PMBusCoefficients {
     int32_t R;     /* exponent */
 } PMBusCoefficients;
 
+/**
+ * VOUT_Mode bit fields
+ */
+typedef struct PMBusVoutMode {
+    uint8_t  mode:3;
+    int8_t   exp:5;
+} PMBusVoutMode;
+
 /**
  * Convert sensor values to direct mode format
  *
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 3/5] hw/i2c: pmbus: add fan support

[Titus Rwantare]

Reviewed-by: Stephen Longfield <slongfield@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/i2c/pmbus_device.c         | 176 ++++++++++++++++++++++++++++++++++
 include/hw/i2c/pmbus_device.h |   1 +
 2 files changed, 177 insertions(+)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index 02647769cd..bb42e410b4 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -490,6 +490,54 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
         }
         break;
 
+    case PMBUS_FAN_CONFIG_1_2:            /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send8(pmdev, pmdev->pages[index].fan_config_1_2);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_1:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].fan_command_1);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_2:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].fan_command_2);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_FAN_CONFIG_3_4:            /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send8(pmdev, pmdev->pages[index].fan_config_3_4);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_3:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].fan_command_3);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_4:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].fan_command_4);
+        } else {
+            goto passthough;
+        }
+        break;
+
     case PMBUS_VOUT_OV_FAULT_LIMIT:       /* R/W word */
         if (pmdev->pages[index].page_flags & PB_HAS_VOUT) {
             pmbus_send16(pmdev, pmdev->pages[index].vout_ov_fault_limit);
@@ -800,6 +848,22 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
         pmbus_send8(pmdev, pmdev->pages[index].status_mfr_specific);
         break;
 
+    case PMBUS_STATUS_FANS_1_2:           /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send8(pmdev, pmdev->pages[index].status_fans_1_2);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_STATUS_FANS_3_4:           /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send8(pmdev, pmdev->pages[index].status_fans_3_4);
+        } else {
+            goto passthough;
+        }
+        break;
+
     case PMBUS_READ_EIN:                  /* Read-Only block 5 bytes */
         if (pmdev->pages[index].page_flags & PB_HAS_EIN) {
             pmbus_send(pmdev, pmdev->pages[index].read_ein, 5);
@@ -872,6 +936,54 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
         }
         break;
 
+    case PMBUS_READ_FAN_SPEED_1:          /* Read-Only word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_1);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_READ_FAN_SPEED_2:          /* Read-Only word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_2);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_READ_FAN_SPEED_3:          /* Read-Only word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_3);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_READ_FAN_SPEED_4:          /* Read-Only word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_4);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_READ_DUTY_CYCLE:           /* Read-Only word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].read_duty_cycle);
+        } else {
+            goto passthough;
+        }
+        break;
+
+    case PMBUS_READ_FREQUENCY:            /* Read-Only word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send16(pmdev, pmdev->pages[index].read_frequency);
+        } else {
+            goto passthough;
+        }
+        break;
+
     case PMBUS_READ_POUT:                 /* Read-Only word */
         if (pmdev->pages[index].page_flags & PB_HAS_POUT) {
             pmbus_send16(pmdev, pmdev->pages[index].read_pout);
@@ -1295,6 +1407,54 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
         }
         break;
 
+    case PMBUS_FAN_CONFIG_1_2:            /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmdev->pages[index].fan_config_1_2 = pmbus_receive8(pmdev);
+        } else {
+            goto passthrough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_1:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmdev->pages[index].fan_command_1 = pmbus_receive16(pmdev);
+        } else {
+            goto passthrough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_2:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmdev->pages[index].fan_command_2 = pmbus_receive16(pmdev);
+        } else {
+            goto passthrough;
+        }
+        break;
+
+    case PMBUS_FAN_CONFIG_3_4:            /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmdev->pages[index].fan_config_3_4 = pmbus_receive8(pmdev);
+        } else {
+            goto passthrough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_3:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmdev->pages[index].fan_command_3 = pmbus_receive16(pmdev);
+        } else {
+            goto passthrough;
+        }
+        break;
+
+    case PMBUS_FAN_COMMAND_4:             /* R/W word */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmdev->pages[index].fan_command_4 = pmbus_receive16(pmdev);
+        } else {
+            goto passthrough;
+        }
+        break;
+
     case PMBUS_VOUT_OV_FAULT_LIMIT:       /* R/W word */
         if (pmdev->pages[index].page_flags & PB_HAS_VOUT) {
             pmdev->pages[index].vout_ov_fault_limit = pmbus_receive16(pmdev);
@@ -1600,6 +1760,22 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
         pmdev->pages[index].status_mfr_specific = pmbus_receive8(pmdev);
         break;
 
+    case PMBUS_STATUS_FANS_1_2:           /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send8(pmdev, pmdev->pages[index].status_fans_1_2);
+        } else {
+            goto passthrough;
+        }
+        break;
+
+    case PMBUS_STATUS_FANS_3_4:           /* R/W byte */
+        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
+            pmbus_send8(pmdev, pmdev->pages[index].status_fans_3_4);
+        } else {
+            goto passthrough;
+        }
+        break;
+
     case PMBUS_PAGE_PLUS_READ:            /* Block Read-only */
     case PMBUS_CAPABILITY:                /* Read-Only byte */
     case PMBUS_COEFFICIENTS:              /* Read-only block 5 bytes */
diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index 2e95164aa1..ad431bdc7c 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -258,6 +258,7 @@ OBJECT_DECLARE_TYPE(PMBusDevice, PMBusDeviceClass,
 #define PB_HAS_TEMP2               BIT_ULL(41)
 #define PB_HAS_TEMP3               BIT_ULL(42)
 #define PB_HAS_TEMP_RATING         BIT_ULL(43)
+#define PB_HAS_FAN                 BIT_ULL(44)
 #define PB_HAS_MFR_INFO            BIT_ULL(50)
 #define PB_HAS_STATUS_MFR_SPECIFIC BIT_ULL(51)
 
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 4/5] hw/i2c: pmbus: block uninitialised string reads

[Titus Rwantare]

Devices models calling pmbus_send_string can't be relied upon to
send a non-zero pointer. This logs an error and doesn't segfault.

Reviewed-by: Patrick Venture <venture@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/i2c/pmbus_device.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index bb42e410b4..18e629eaac 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -94,6 +94,13 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
 
 void pmbus_send_string(PMBusDevice *pmdev, const char *data)
 {
+    if (!data) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: %s: uninitialised read from 0x%02x\n",
+                      __func__, DEVICE(pmdev)->canonical_path, pmdev->code);
+        return;
+    }
+
     size_t len = strlen(data);
     g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
     pmdev->out_buf[len + pmdev->out_buf_len] = len;
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 5/5] hw/i2c: pmbus: add VCAP register

[Titus Rwantare]

VCAP is a register for devices with energy storage capacitors.

Reviewed-by: Benjamin Streb <bstreb@google.com>
Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/i2c/pmbus_device.c         | 8 ++++++++
 include/hw/i2c/pmbus_device.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index 18e629eaac..ef0314a913 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -903,6 +903,14 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
         }
         break;
 
+    case PMBUS_READ_VCAP:                 /* Read-Only word */
+        if (pmdev->pages[index].page_flags & PB_HAS_VCAP) {
+            pmbus_send16(pmdev, pmdev->pages[index].read_vcap);
+        } else {
+            goto passthough;
+        }
+        break;
+
     case PMBUS_READ_VOUT:                 /* Read-Only word */
         if (pmdev->pages[index].page_flags & PB_HAS_VOUT) {
             pmbus_send16(pmdev, pmdev->pages[index].read_vout);
diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index ad431bdc7c..f195c11384 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -243,6 +243,7 @@ OBJECT_DECLARE_TYPE(PMBusDevice, PMBusDeviceClass,
 #define PB_HAS_VIN_RATING          BIT_ULL(13)
 #define PB_HAS_VOUT_RATING         BIT_ULL(14)
 #define PB_HAS_VOUT_MODE           BIT_ULL(15)
+#define PB_HAS_VCAP                BIT_ULL(16)
 #define PB_HAS_IOUT                BIT_ULL(21)
 #define PB_HAS_IIN                 BIT_ULL(22)
 #define PB_HAS_IOUT_RATING         BIT_ULL(23)
-- 
2.40.0.rc1.284.g88254d51c5-goog

Re: [PATCH 4/5] hw/i2c: pmbus: block uninitialised string reads

[Philippe Mathieu-Daudé]

On 22/3/23 18:55, Titus Rwantare wrote:
> Devices models calling pmbus_send_string can't be relied upon to
> send a non-zero pointer. This logs an error and doesn't segfault.
> 
> Reviewed-by: Patrick Venture <venture@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>   hw/i2c/pmbus_device.c | 7 +++++++
>   1 file changed, 7 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/5] PMBus fixes and new functions

[Philippe Mathieu-Daudé]

On 22/3/23 18:55, Titus Rwantare wrote:
> This patch series contains fixes and improvements to PMBus support in QEMU.

> Fixes:
>     - String read now handles now logs an error when passed an empty string
> 

> Titus Rwantare (5):

>    hw/i2c: pmbus: block uninitialised string reads

Patch #4 queued for 8.0-rc3, thanks.

Re: [PATCH 1/5] hw/i2c: pmbus add support for block receive

[Corey Minyard]

It's generally frowned upon to have empty descriptions, some rationale
would be helpful.  For instance, you remove a length check from the send
string, why did you do that?

Any why is this being added?  What's it supporting?

-corey

On Wed, Mar 22, 2023 at 05:55:09PM +0000, Titus Rwantare wrote:
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>  hw/i2c/pmbus_device.c         | 30 +++++++++++++++++++++++++++++-
>  include/hw/i2c/pmbus_device.h |  7 +++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index c3d6046784..02647769cd 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
>  void pmbus_send_string(PMBusDevice *pmdev, const char *data)
>  {
>      size_t len = strlen(data);
> -    g_assert(len > 0);
>      g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
>      pmdev->out_buf[len + pmdev->out_buf_len] = len;
>  
> @@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
>      pmdev->out_buf_len += len + 1;
>  }
>  
> +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len)
> +{
> +    /* dest may contain data from previous writes */
> +    memset(dest, 0, len);
> +
> +    /* Exclude command code from return value */
> +    pmdev->in_buf++;
> +    pmdev->in_buf_len--;
> +
> +    /* The byte after the command code denotes the length */
> +    uint8_t sent_len = pmdev->in_buf[0];
> +
> +    if (sent_len != pmdev->in_buf_len - 1) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: length mismatch. Expected %d bytes, got %d bytes\n",
> +                      __func__, sent_len, pmdev->in_buf_len - 1);
> +    }
> +
> +    /* exclude length byte */
> +    pmdev->in_buf++;
> +    pmdev->in_buf_len--;
> +
> +    if (pmdev->in_buf_len < len) {
> +        len = pmdev->in_buf_len;
> +    }
> +    memcpy(dest, pmdev->in_buf, len);
> +    return len;
> +}
> +
>  
>  static uint64_t pmbus_receive_uint(PMBusDevice *pmdev)
>  {
> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 93f5d57c9d..7dc00cc4d9 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t data);
>   */
>  void pmbus_send_string(PMBusDevice *state, const char *data);
>  
> +/**
> + * @brief Receive data sent with Block Write.
> + * @param dest - memory with enough capacity to receive the write
> + * @param len - the capacity of dest
> + */
> +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len);
> +
>  /**
>   * @brief Receive data over PMBus
>   * These methods help track how much data is being received over PMBus
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
> 

Re: [PATCH 2/5] hw/i2c: pmbus: add vout mode bitfields

[Corey Minyard]

I almost never say this, as patches are usually too large :), but it
would be nice if you combined this with the patch that uses the
structure so we can see what it's used for.  Especially since that patch
is several patches down the line.

-corey

On Wed, Mar 22, 2023 at 05:55:10PM +0000, Titus Rwantare wrote:
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>  include/hw/i2c/pmbus_device.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 7dc00cc4d9..2e95164aa1 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -444,6 +444,14 @@ typedef struct PMBusCoefficients {
>      int32_t R;     /* exponent */
>  } PMBusCoefficients;
>  
> +/**
> + * VOUT_Mode bit fields
> + */
> +typedef struct PMBusVoutMode {
> +    uint8_t  mode:3;
> +    int8_t   exp:5;
> +} PMBusVoutMode;
> +
>  /**
>   * Convert sensor values to direct mode format
>   *
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
> 

Re: [PATCH 3/5] hw/i2c: pmbus: add fan support

[Corey Minyard]

Empty description, but the code itself looks ok.

Acked-by: Corey Minyard <cminyard@mvista.com>

On Wed, Mar 22, 2023 at 05:55:11PM +0000, Titus Rwantare wrote:
> Reviewed-by: Stephen Longfield <slongfield@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>  hw/i2c/pmbus_device.c         | 176 ++++++++++++++++++++++++++++++++++
>  include/hw/i2c/pmbus_device.h |   1 +
>  2 files changed, 177 insertions(+)
> 
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index 02647769cd..bb42e410b4 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -490,6 +490,54 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>          }
>          break;
>  
> +    case PMBUS_FAN_CONFIG_1_2:            /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send8(pmdev, pmdev->pages[index].fan_config_1_2);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_1:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].fan_command_1);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_2:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].fan_command_2);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_CONFIG_3_4:            /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send8(pmdev, pmdev->pages[index].fan_config_3_4);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_3:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].fan_command_3);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_4:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].fan_command_4);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
>      case PMBUS_VOUT_OV_FAULT_LIMIT:       /* R/W word */
>          if (pmdev->pages[index].page_flags & PB_HAS_VOUT) {
>              pmbus_send16(pmdev, pmdev->pages[index].vout_ov_fault_limit);
> @@ -800,6 +848,22 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>          pmbus_send8(pmdev, pmdev->pages[index].status_mfr_specific);
>          break;
>  
> +    case PMBUS_STATUS_FANS_1_2:           /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send8(pmdev, pmdev->pages[index].status_fans_1_2);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_STATUS_FANS_3_4:           /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send8(pmdev, pmdev->pages[index].status_fans_3_4);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
>      case PMBUS_READ_EIN:                  /* Read-Only block 5 bytes */
>          if (pmdev->pages[index].page_flags & PB_HAS_EIN) {
>              pmbus_send(pmdev, pmdev->pages[index].read_ein, 5);
> @@ -872,6 +936,54 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>          }
>          break;
>  
> +    case PMBUS_READ_FAN_SPEED_1:          /* Read-Only word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_1);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_READ_FAN_SPEED_2:          /* Read-Only word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_2);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_READ_FAN_SPEED_3:          /* Read-Only word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_3);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_READ_FAN_SPEED_4:          /* Read-Only word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].read_fan_speed_4);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_READ_DUTY_CYCLE:           /* Read-Only word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].read_duty_cycle);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
> +    case PMBUS_READ_FREQUENCY:            /* Read-Only word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send16(pmdev, pmdev->pages[index].read_frequency);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
>      case PMBUS_READ_POUT:                 /* Read-Only word */
>          if (pmdev->pages[index].page_flags & PB_HAS_POUT) {
>              pmbus_send16(pmdev, pmdev->pages[index].read_pout);
> @@ -1295,6 +1407,54 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
>          }
>          break;
>  
> +    case PMBUS_FAN_CONFIG_1_2:            /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmdev->pages[index].fan_config_1_2 = pmbus_receive8(pmdev);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_1:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmdev->pages[index].fan_command_1 = pmbus_receive16(pmdev);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_2:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmdev->pages[index].fan_command_2 = pmbus_receive16(pmdev);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_CONFIG_3_4:            /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmdev->pages[index].fan_config_3_4 = pmbus_receive8(pmdev);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_3:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmdev->pages[index].fan_command_3 = pmbus_receive16(pmdev);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
> +    case PMBUS_FAN_COMMAND_4:             /* R/W word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmdev->pages[index].fan_command_4 = pmbus_receive16(pmdev);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
>      case PMBUS_VOUT_OV_FAULT_LIMIT:       /* R/W word */
>          if (pmdev->pages[index].page_flags & PB_HAS_VOUT) {
>              pmdev->pages[index].vout_ov_fault_limit = pmbus_receive16(pmdev);
> @@ -1600,6 +1760,22 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
>          pmdev->pages[index].status_mfr_specific = pmbus_receive8(pmdev);
>          break;
>  
> +    case PMBUS_STATUS_FANS_1_2:           /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send8(pmdev, pmdev->pages[index].status_fans_1_2);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
> +    case PMBUS_STATUS_FANS_3_4:           /* R/W byte */
> +        if (pmdev->pages[index].page_flags & PB_HAS_FAN) {
> +            pmbus_send8(pmdev, pmdev->pages[index].status_fans_3_4);
> +        } else {
> +            goto passthrough;
> +        }
> +        break;
> +
>      case PMBUS_PAGE_PLUS_READ:            /* Block Read-only */
>      case PMBUS_CAPABILITY:                /* Read-Only byte */
>      case PMBUS_COEFFICIENTS:              /* Read-only block 5 bytes */
> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 2e95164aa1..ad431bdc7c 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -258,6 +258,7 @@ OBJECT_DECLARE_TYPE(PMBusDevice, PMBusDeviceClass,
>  #define PB_HAS_TEMP2               BIT_ULL(41)
>  #define PB_HAS_TEMP3               BIT_ULL(42)
>  #define PB_HAS_TEMP_RATING         BIT_ULL(43)
> +#define PB_HAS_FAN                 BIT_ULL(44)
>  #define PB_HAS_MFR_INFO            BIT_ULL(50)
>  #define PB_HAS_STATUS_MFR_SPECIFIC BIT_ULL(51)
>  
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
> 

Re: [PATCH 4/5] hw/i2c: pmbus: block uninitialised string reads

[Corey Minyard]

On Wed, Mar 22, 2023 at 05:55:12PM +0000, Titus Rwantare wrote:
> Devices models calling pmbus_send_string can't be relied upon to
> send a non-zero pointer. This logs an error and doesn't segfault.

Yes, a good idea.

Acked-by: Corey Minyard <cminyard@mvista.com>

> 
> Reviewed-by: Patrick Venture <venture@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>  hw/i2c/pmbus_device.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index bb42e410b4..18e629eaac 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -94,6 +94,13 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
>  
>  void pmbus_send_string(PMBusDevice *pmdev, const char *data)
>  {
> +    if (!data) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: %s: uninitialised read from 0x%02x\n",
> +                      __func__, DEVICE(pmdev)->canonical_path, pmdev->code);
> +        return;
> +    }
> +
>      size_t len = strlen(data);
>      g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
>      pmdev->out_buf[len + pmdev->out_buf_len] = len;
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
> 

Re: [PATCH 5/5] hw/i2c: pmbus: add VCAP register

[Corey Minyard]

On Wed, Mar 22, 2023 at 05:55:13PM +0000, Titus Rwantare wrote:
> VCAP is a register for devices with energy storage capacitors.

Acked-by: Corey MInyard <cminyard@mvista.com>

> 
> Reviewed-by: Benjamin Streb <bstreb@google.com>
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>  hw/i2c/pmbus_device.c         | 8 ++++++++
>  include/hw/i2c/pmbus_device.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index 18e629eaac..ef0314a913 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -903,6 +903,14 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>          }
>          break;
>  
> +    case PMBUS_READ_VCAP:                 /* Read-Only word */
> +        if (pmdev->pages[index].page_flags & PB_HAS_VCAP) {
> +            pmbus_send16(pmdev, pmdev->pages[index].read_vcap);
> +        } else {
> +            goto passthough;
> +        }
> +        break;
> +
>      case PMBUS_READ_VOUT:                 /* Read-Only word */
>          if (pmdev->pages[index].page_flags & PB_HAS_VOUT) {
>              pmbus_send16(pmdev, pmdev->pages[index].read_vout);
> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index ad431bdc7c..f195c11384 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -243,6 +243,7 @@ OBJECT_DECLARE_TYPE(PMBusDevice, PMBusDeviceClass,
>  #define PB_HAS_VIN_RATING          BIT_ULL(13)
>  #define PB_HAS_VOUT_RATING         BIT_ULL(14)
>  #define PB_HAS_VOUT_MODE           BIT_ULL(15)
> +#define PB_HAS_VCAP                BIT_ULL(16)
>  #define PB_HAS_IOUT                BIT_ULL(21)
>  #define PB_HAS_IIN                 BIT_ULL(22)
>  #define PB_HAS_IOUT_RATING         BIT_ULL(23)
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
> 

Re: [PATCH 2/5] hw/i2c: pmbus: add vout mode bitfields

[Corey Minyard]

On Thu, Mar 30, 2023 at 11:20:11AM -0500, Corey Minyard wrote:
> I almost never say this, as patches are usually too large :), but it
> would be nice if you combined this with the patch that uses the
> structure so we can see what it's used for.  Especially since that patch
> is several patches down the line.

Actually, in re-reviewing, I don't see this used at all.  Is there
something I'm missing?

> 
> -corey
> 
> On Wed, Mar 22, 2023 at 05:55:10PM +0000, Titus Rwantare wrote:
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Signed-off-by: Titus Rwantare <titusr@google.com>
> > ---
> >  include/hw/i2c/pmbus_device.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> > index 7dc00cc4d9..2e95164aa1 100644
> > --- a/include/hw/i2c/pmbus_device.h
> > +++ b/include/hw/i2c/pmbus_device.h
> > @@ -444,6 +444,14 @@ typedef struct PMBusCoefficients {
> >      int32_t R;     /* exponent */
> >  } PMBusCoefficients;
> >  
> > +/**
> > + * VOUT_Mode bit fields
> > + */
> > +typedef struct PMBusVoutMode {
> > +    uint8_t  mode:3;
> > +    int8_t   exp:5;
> > +} PMBusVoutMode;
> > +
> >  /**
> >   * Convert sensor values to direct mode format
> >   *
> > -- 
> > 2.40.0.rc1.284.g88254d51c5-goog
> > 

Re: [PATCH 1/5] hw/i2c: pmbus add support for block receive

[Titus Rwantare]

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

Apologies. I've updated the commit descriptions and added a sensor using
this code in v2.

While doing block receive I discovered that it is valid behaviour to erase
a field and have it be an empty string.

-Titus

On Thu, 30 Mar 2023 at 09:18, Corey Minyard <minyard@acm.org> wrote:

> It's generally frowned upon to have empty descriptions, some rationale
> would be helpful.  For instance, you remove a length check from the send
> string, why did you do that?
>
> Any why is this being added?  What's it supporting?
>
> -corey
>
> On Wed, Mar 22, 2023 at 05:55:09PM +0000, Titus Rwantare wrote:
> > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > Signed-off-by: Titus Rwantare <titusr@google.com>
> > ---
> >  hw/i2c/pmbus_device.c         | 30 +++++++++++++++++++++++++++++-
> >  include/hw/i2c/pmbus_device.h |  7 +++++++
> >  2 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> > index c3d6046784..02647769cd 100644
> > --- a/hw/i2c/pmbus_device.c
> > +++ b/hw/i2c/pmbus_device.c
> > @@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
> >  void pmbus_send_string(PMBusDevice *pmdev, const char *data)
> >  {
> >      size_t len = strlen(data);
> > -    g_assert(len > 0);
> >      g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
> >      pmdev->out_buf[len + pmdev->out_buf_len] = len;
> >
> > @@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const
> char *data)
> >      pmdev->out_buf_len += len + 1;
> >  }
> >
> > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t
> len)
> > +{
> > +    /* dest may contain data from previous writes */
> > +    memset(dest, 0, len);
> > +
> > +    /* Exclude command code from return value */
> > +    pmdev->in_buf++;
> > +    pmdev->in_buf_len--;
> > +
> > +    /* The byte after the command code denotes the length */
> > +    uint8_t sent_len = pmdev->in_buf[0];
> > +
> > +    if (sent_len != pmdev->in_buf_len - 1) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: length mismatch. Expected %d bytes, got %d
> bytes\n",
> > +                      __func__, sent_len, pmdev->in_buf_len - 1);
> > +    }
> > +
> > +    /* exclude length byte */
> > +    pmdev->in_buf++;
> > +    pmdev->in_buf_len--;
> > +
> > +    if (pmdev->in_buf_len < len) {
> > +        len = pmdev->in_buf_len;
> > +    }
> > +    memcpy(dest, pmdev->in_buf, len);
> > +    return len;
> > +}
> > +
> >
> >  static uint64_t pmbus_receive_uint(PMBusDevice *pmdev)
> >  {
> > diff --git a/include/hw/i2c/pmbus_device.h
> b/include/hw/i2c/pmbus_device.h
> > index 93f5d57c9d..7dc00cc4d9 100644
> > --- a/include/hw/i2c/pmbus_device.h
> > +++ b/include/hw/i2c/pmbus_device.h
> > @@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t
> data);
> >   */
> >  void pmbus_send_string(PMBusDevice *state, const char *data);
> >
> > +/**
> > + * @brief Receive data sent with Block Write.
> > + * @param dest - memory with enough capacity to receive the write
> > + * @param len - the capacity of dest
> > + */
> > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t
> len);
> > +
> >  /**
> >   * @brief Receive data over PMBus
> >   * These methods help track how much data is being received over PMBus
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >
>

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