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

[Patrick Venture <venture@google.com>]

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

On Mon, Jan 10, 2022 at 11:30 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 1/10/22 18:14, Patrick Venture wrote:
> > On Mon, Jan 10, 2022 at 1:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org
> > <mailto:f4bug@amsat.org>> wrote:
> >
> >     Hi Patrick,
> >
> >     On 1/8/22 04:04, Patrick Venture wrote:
> >     > From: Hao Wu <wuhaotsh@google.com <mailto: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
> >     <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 <wuhaotsh@google.com
> >     <mailto:wuhaotsh@google.com>>
> >     > Reviewed-by: Doug Evans <dje@google.com <mailto: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?
> >
> >
> > 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 :)
>

Ha, thanks! :) I didn't notice Hao had done that. I'll make it consistent.

>
> >     > +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.
>

Thanks, it seems like the type of change that was best as sweeping vs a
one-off.


>
> >     > +                        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.
>

Ok, so split the patches into two commits in the same mailing thing.  That
makes sense.  I'll set that up for the v2.

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