Hi Patrick,
On 1/8/22 04:04, Patrick Venture wrote:
> From: Hao Wu <wuhao...@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 <wuhao...@google.com>
> Reviewed-by: Doug Evans <d...@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.
