On 1/10/22 18:14, Patrick Venture wrote:
> On Mon, Jan 10, 2022 at 1:35 AM Philippe Mathieu-Daudé <f4...@amsat.org
> <mailto:f4...@amsat.org>> wrote:
>
> Hi Patrick,
>
> On 1/8/22 04:04, Patrick Venture wrote:
> > From: Hao Wu <wuhao...@google.com <mailto: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
> <https://lkml.org/lkml/2020/12/11/968>
> > Register Map:
> > https://developer.amd.com/wp-content/resources/56255_3_03.PDF
> <https://developer.amd.com/wp-content/resources/56255_3_03.PDF>
> > (Chapter 6)
> >
> > Signed-off-by: Hao Wu <wuhao...@google.com
> <mailto:wuhao...@google.com>>
> > Reviewed-by: Doug Evans <d...@google.com <mailto: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?
>
>
> I haven't seen this suffix before, is this a style you'd like to start
> seeing?
Well you used it few lines earlier, in the previous definition:
SBTSI_TEMP_UNIT_IN_MILLIDEGREE :)
> > +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"?
>
>
> We can, but that's non-standard. As I recall most temperature sensors
> in hw/sensors are in millidegrees, but none have unit suffixes.
I tend to agree, but this is not obvious to everybody.
I once started sanitizing / enforcing this but never got there:
https://www.mail-archive.com/qemu-block@nongnu.org/msg65192.html
So I don't mind for your patch, if I keep looking at temp sensors
I'll clean up on top later.
> > + 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
> <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".
>
>
> Ack.
>
>
>
> > +/* 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.
>
>
> v2 will come with the header split. I can split the qtests into a
> separate patch. Was your point because the patches are applied to
> different staging trees? Because I would think it ideal to have the
> qtests and the corresponding code applied together so that it's easy to
> know it's working. If this isn't standard, I can definitely split them.
If split, both patches certainly should be queued consecutively via the
same tree. I asked the split because I'm OK to give a R-b tag for the
hw/ part, but am not sure about the tests/ part. Possibly others would
be OK to review the qtest but would be unsure about the hw/ part.
