Re: [PATCH] hw/sensor: Add SB-TSI Temperature Sensor Interface

[Philippe Mathieu-Daudé]

Hi Patrick,

On 1/8/22 04:04, Patrick Venture wrote:
> From: Hao Wu <wuhaotsh@google.com>
> 
> SB Temperature Sensor Interface (SB-TSI) is an SMBus compatible
> interface that reports AMD SoC's Ttcl (normalized temperature),
> and resembles a typical 8-pin remote temperature sensor's I2C interface
> to BMC.
> 
> This patch implements a basic AMD SB-TSI sensor that is
> compatible with the open-source data sheet from AMD and Linux
> kernel driver.
> 
> Reference:
> Linux kernel driver:
> https://lkml.org/lkml/2020/12/11/968
> Register Map:
> https://developer.amd.com/wp-content/resources/56255_3_03.PDF
> (Chapter 6)
> 
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Doug Evans <dje@google.com>
> ---
>  hw/sensor/Kconfig            |   4 +
>  hw/sensor/meson.build        |   1 +
>  hw/sensor/tmp_sbtsi.c        | 393 +++++++++++++++++++++++++++++++++++
>  hw/sensor/trace-events       |   5 +
>  hw/sensor/trace.h            |   1 +
>  meson.build                  |   1 +

>  tests/qtest/meson.build      |   1 +
>  tests/qtest/tmp_sbtsi-test.c | 180 ++++++++++++++++

Up to Thomas for qtest, but I'd rather split in 2 patches since
different set of maintainers / reviewers.

> +++ b/hw/sensor/tmp_sbtsi.c
> @@ -0,0 +1,393 @@
> +/*
> + * AMD SBI Temperature Sensor Interface (SB-TSI)
> + *
> + * Copyright 2021 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/i2c/smbus_slave.h"
> +#include "hw/irq.h"
> +#include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "trace.h"
> +
> +#define TYPE_SBTSI "sbtsi"

If you add include/hw/sensor/sbtsi.h, please define the type there, ...

> +/*
> + * SB-TSI registers only support SMBus byte data access. "_INT" registers are
> + * the integer part of a temperature value or limit, and "_DEC" registers are
> + * corresponding decimal parts.
> + */
> +#define SBTSI_REG_TEMP_INT      0x01 /* RO */
> +#define SBTSI_REG_STATUS        0x02 /* RO */
> +#define SBTSI_REG_CONFIG        0x03 /* RO */
> +#define SBTSI_REG_TEMP_HIGH_INT 0x07 /* RW */
> +#define SBTSI_REG_TEMP_LOW_INT  0x08 /* RW */
> +#define SBTSI_REG_CONFIG_WR     0x09 /* RW */
> +#define SBTSI_REG_TEMP_DEC      0x10 /* RO */
> +#define SBTSI_REG_TEMP_HIGH_DEC 0x13 /* RW */
> +#define SBTSI_REG_TEMP_LOW_DEC  0x14 /* RW */
> +#define SBTSI_REG_ALERT_CONFIG  0xBF /* RW */
> +#define SBTSI_REG_MAN           0xFE /* RO */
> +#define SBTSI_REG_REV           0xFF /* RO */
> +
> +#define SBTSI_STATUS_HIGH_ALERT BIT(4)
> +#define SBTSI_STATUS_LOW_ALERT  BIT(3)
> +#define SBTSI_CONFIG_ALERT_MASK BIT(7)
> +#define SBTSI_ALARM_EN          BIT(0)

beside these definitions can be share with qtests.

> +/* The temperature we stored are in units of 0.125 degrees. */
> +#define SBTSI_TEMP_UNIT_IN_MILLIDEGREE 125
> +
> +/*
> + * The integer part and decimal of the temperature both 8 bits.
> + * Only the top 3 bits of the decimal parts are used.
> + * So the max temperature is (2^8-1) + (2^3-1)/8 = 255.875 degrees.
> + */
> +#define SBTSI_TEMP_MAX 255875

Nitpicking, use _IN_MILLIDEGREE suffix?

> +static void sbtsi_init(Object *obj)
> +{
> +    SBTSIState *s = SBTSI(obj);
> +
> +    /* Current temperature in millidegrees. */
> +    object_property_add(obj, "temperature", "uint32",

Can we explicit as "temperature_uC"?

> +                        sbtsi_get_temperature, sbtsi_set_temperature,
> +                        NULL, NULL);
> +    qdev_init_gpio_out_named(DEVICE(obj), &s->alarm, SBTSI_ALARM_L, 0);
> +}
> +
> +static void sbtsi_class_init(ObjectClass *klass, void *data)
> +{
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SMBusDeviceClass *k = SMBUS_DEVICE_CLASS(klass);
> +
> +    dc->desc = "SB-TSI Temperature Sensor";
> +    dc->vmsd = &vmstate_sbtsi;
> +    k->write_data = sbtsi_write_data;
> +    k->receive_byte = sbtsi_read_byte;
> +    rc->phases.enter = sbtsi_enter_reset;
> +    rc->phases.hold = sbtsi_hold_reset;

If my previous patch [*] were in, I'd have asked for:

       set_bit(DEVICE_CATEGORY_SENSOR, dc->categories);

But since it isn't, I'll keep an eye on which goes in first.

[*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg847088.html

> diff --git a/tests/qtest/tmp_sbtsi-test.c b/tests/qtest/tmp_sbtsi-test.c
> new file mode 100644
> index 0000000000..7f5fafacc7
> --- /dev/null
> +++ b/tests/qtest/tmp_sbtsi-test.c
> @@ -0,0 +1,180 @@
> +/*
> + * QTests for the SBTSI temperature sensor
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "libqtest-single.h"
> +#include "libqos/qgraph.h"
> +#include "libqos/i2c.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qemu/bitops.h"
> +
> +#define TEST_ID   "sbtsi-test"
> +#define TEST_ADDR (0x4c)
> +
> +/*
> + * SB-TSI registers only support SMBus byte data access. "_INT" registers are
> + * the integer part of a temperature value or limit, and "_DEC" registers are
> + * corresponding decimal parts.
> + */
> +#define REG_TEMP_INT      0x01 /* RO */
> +#define REG_STATUS        0x02 /* RO */
> +#define REG_CONFIG        0x03 /* RO */
> +#define REG_TEMP_HIGH_INT 0x07 /* RW */
> +#define REG_TEMP_LOW_INT  0x08 /* RW */
> +#define REG_CONFIG_WR     0x09 /* RW */
> +#define REG_TEMP_DEC      0x10 /* RO */
> +#define REG_TEMP_HIGH_DEC 0x13 /* RW */
> +#define REG_TEMP_LOW_DEC  0x14 /* RW */
> +#define REG_ALERT_CONFIG  0xBF /* RW */
> +
> +#define STATUS_HIGH_ALERT BIT(4)
> +#define STATUS_LOW_ALERT  BIT(3)
> +#define CONFIG_ALERT_MASK BIT(7)
> +#define ALARM_EN          BIT(0)

This is the part that could be shared in "include/hw/sensor/sbtsi.h".

> +/* The temperature stored are in units of 0.125 degrees. */
> +#define TEMP_UNIT_IN_MILLIDEGREE 125
> +#define LIMIT_LOW (10500)
> +#define LIMIT_HIGH (55125)

Nitpicking again, _IN_MILLIDEGREE suffix for coherency?

> +static uint32_t qmp_sbtsi_get_temperature(const char *id)
> +{
> +    QDict *response;
> +    int ret;
> +
> +    response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
> +                   "'property': 'temperature' } }", id);

If renamed earlier, here 'temperature_uC'.

> +    g_assert(qdict_haskey(response, "return"));
> +    ret = (uint32_t)qdict_get_int(response, "return");
> +    qobject_unref(response);
> +    return ret;
> +}
> +
> +static void qmp_sbtsi_set_temperature(const char *id, uint32_t value)
> +{
> +    QDict *response;
> +
> +    response = qmp("{ 'execute': 'qom-set', 'arguments': { 'path': %s, "
> +                   "'property': 'temperature', 'value': %d } }", id, value);
> +    g_assert(qdict_haskey(response, "return"));
> +    qobject_unref(response);
> +}

Thanks,

Phil.