Re: [PATCH 1/3] hw/sensor: add MAX31790 fan controller
On Wed, 12 Jan 2022 at 00:25, Titus Rwantare wrote:
>
> Signed-off-by: Titus Rwantare
> Reviewed-by: Hao Wu
Hi; thanks for this patchset. For future revisions, could you
make sure you send a cover-letter for patchsets that have
more than one patch? It helps to keep the automated tooling
from getting confused.
> ---
> MAINTAINERS | 8 +-
> hw/arm/Kconfig| 1 +
> hw/sensor/Kconfig | 4 +
> hw/sensor/max31790_fan_ctrl.c | 454 ++
> hw/sensor/meson.build | 1 +
> include/hw/sensor/max31790_fan_ctrl.h | 93 ++
> 6 files changed, 560 insertions(+), 1 deletion(-)
> create mode 100644 hw/sensor/max31790_fan_ctrl.c
> create mode 100644 include/hw/sensor/max31790_fan_ctrl.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c98a61caee..0791b6be42 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2304,6 +2304,12 @@ F: hw/timer/mips_gictimer.c
> F: include/hw/intc/mips_gic.h
> F: include/hw/timer/mips_gictimer.h
>
> +MAX31790 Fan controller
> +M: Titus Rwantare
> +S: Maintained
> +F: hw/sensor/max31790_fan_ctrl.c
> +F: include/hw/sensor/max31790_fan_ctrl.h
> +
> Subsystems
> --
> Overall Audio backends
> @@ -2798,7 +2804,7 @@ R: Paolo Bonzini
> R: Bandan Das
> R: Stefan Hajnoczi
> R: Thomas Huth
> -R: Darren Kenny
> +R: Darren Kenny
Why did this line get changed ? It looks like maybe there was
a trailing space that got deleted. If you want to do that kind
of cleanup it's best done as a separate patch.
> R: Qiuhao Li
> S: Maintained
> F: tests/qtest/fuzz/
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index e652590943..00bfbaf1c4 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -393,6 +393,7 @@ config NPCM7XX
> select SMBUS
> select AT24C # EEPROM
> select MAX34451
> +select MAX31790
> select PL310 # cache controller
> select PMBUS
> select SERIAL
> diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
> index 9c8a049b06..54d269d642 100644
> --- a/hw/sensor/Kconfig
> +++ b/hw/sensor/Kconfig
> @@ -21,3 +21,7 @@ config ADM1272
> config MAX34451
> bool
> depends on I2C
> +
> +config MAX31790
> +bool
> +depends on I2C
> diff --git a/hw/sensor/max31790_fan_ctrl.c b/hw/sensor/max31790_fan_ctrl.c
> new file mode 100644
> index 00..b5334c1130
> --- /dev/null
> +++ b/hw/sensor/max31790_fan_ctrl.c
> @@ -0,0 +1,454 @@
> +/*
> + * MAX31790 Fan controller
> + *
> + * Independently control 6 fans, up to 12 tachometer inputs,
> + * controlled through i2c
> + *
> + * This device model has read/write support for:
> + * - 9-bit pwm through i2c and qom/qmp
> + * - 11-bit tach_count through i2c
> + * - RPM through qom/qmp
> + *
> + * Copyright 2021 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sensor/max31790_fan_ctrl.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "qapi/visitor.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +
> +static uint16_t max31790_get_sr(uint8_t fan_dynamics)
> +{
> +uint16_t sr = 1 << ((fan_dynamics >> 5) & 0b111);
> +return sr > 16 ? 32 : sr;
> +}
> +
> +static void max31790_place_bits(uint16_t *dest, uint16_t byte, uint8_t
> offset)
> +{
> +uint16_t val = *dest;
> +val &= ~(0x00FF << offset);
> +val |= byte << offset;
> +*dest = val;
> +}
> +
> +/*
> + * calculating fan speed
> + * f_TOSC/4 is the clock, 8192Hz
> + * NP = tachometer pulses per revolution (usually 2)
> + * SR = number of periods(pulses) over which the clock ticks are counted
> + * TACH Count = SR x 8192 x 60 / (NP x RPM)
> + * RPM = SR x 8192 x 60 / (NP x TACH count)
> + *
> + * RPM mode - desired tach count is written to TACH Target Count
> + * PWM mode - desired duty cycle is written to PWMOUT Target Duty reg
> + */
> +static void max31790_calculate_tach_count(MAX31790State *ms, uint8_t id)
> +{
> +uint32_t rpm;
> +uint32_t sr = max31790_get_sr(ms->fan_dynamics[id]);
> +ms->pwm_duty_cycle[id] = ms->pwmout[id] >> 7;
> +rpm = (ms->max_rpm[id] * ms->pwm_duty_cycle[id]) / 0x1FF;
> +
> +if (rpm) {
> +ms->tach_count[id] = (sr * MAX31790_CLK_FREQ * 60) /
> + (MAX31790_PULSES_PER_REV * rpm);
This is all being done with 32-bit arithmetic. Is it definitely
never possible for it to overflow ?
> +} else {
> +ms->tach_count[id] = 0;
> +}
> +
> +}
> +
> +if ((ms->command + 1) % 8) {
> +ms->command++;
> +} else {
> +ms->command -= 7;
> +}
You could write this:
ms->command = (ms->command & ~7) | ((ms->command + 1) & 7);
Maybe that's clearer, maybe not -- your choice whether to change it or not.
> +/* assumes that the fans have the same speed range (SR) */
> +static void max31790_get_rpm(Object *obj, Visitor *v, const char
[PATCH 1/3] hw/sensor: add MAX31790 fan controller
Signed-off-by: Titus Rwantare
Reviewed-by: Hao Wu
---
MAINTAINERS | 8 +-
hw/arm/Kconfig| 1 +
hw/sensor/Kconfig | 4 +
hw/sensor/max31790_fan_ctrl.c | 454 ++
hw/sensor/meson.build | 1 +
include/hw/sensor/max31790_fan_ctrl.h | 93 ++
6 files changed, 560 insertions(+), 1 deletion(-)
create mode 100644 hw/sensor/max31790_fan_ctrl.c
create mode 100644 include/hw/sensor/max31790_fan_ctrl.h
diff --git a/MAINTAINERS b/MAINTAINERS
index c98a61caee..0791b6be42 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2304,6 +2304,12 @@ F: hw/timer/mips_gictimer.c
F: include/hw/intc/mips_gic.h
F: include/hw/timer/mips_gictimer.h
+MAX31790 Fan controller
+M: Titus Rwantare
+S: Maintained
+F: hw/sensor/max31790_fan_ctrl.c
+F: include/hw/sensor/max31790_fan_ctrl.h
+
Subsystems
--
Overall Audio backends
@@ -2798,7 +2804,7 @@ R: Paolo Bonzini
R: Bandan Das
R: Stefan Hajnoczi
R: Thomas Huth
-R: Darren Kenny
+R: Darren Kenny
R: Qiuhao Li
S: Maintained
F: tests/qtest/fuzz/
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index e652590943..00bfbaf1c4 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -393,6 +393,7 @@ config NPCM7XX
select SMBUS
select AT24C # EEPROM
select MAX34451
+select MAX31790
select PL310 # cache controller
select PMBUS
select SERIAL
diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
index 9c8a049b06..54d269d642 100644
--- a/hw/sensor/Kconfig
+++ b/hw/sensor/Kconfig
@@ -21,3 +21,7 @@ config ADM1272
config MAX34451
bool
depends on I2C
+
+config MAX31790
+bool
+depends on I2C
diff --git a/hw/sensor/max31790_fan_ctrl.c b/hw/sensor/max31790_fan_ctrl.c
new file mode 100644
index 00..b5334c1130
--- /dev/null
+++ b/hw/sensor/max31790_fan_ctrl.c
@@ -0,0 +1,454 @@
+/*
+ * MAX31790 Fan controller
+ *
+ * Independently control 6 fans, up to 12 tachometer inputs,
+ * controlled through i2c
+ *
+ * This device model has read/write support for:
+ * - 9-bit pwm through i2c and qom/qmp
+ * - 11-bit tach_count through i2c
+ * - RPM through qom/qmp
+ *
+ * Copyright 2021 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sensor/max31790_fan_ctrl.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qapi/visitor.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+
+static uint16_t max31790_get_sr(uint8_t fan_dynamics)
+{
+uint16_t sr = 1 << ((fan_dynamics >> 5) & 0b111);
+return sr > 16 ? 32 : sr;
+}
+
+static void max31790_place_bits(uint16_t *dest, uint16_t byte, uint8_t offset)
+{
+uint16_t val = *dest;
+val &= ~(0x00FF << offset);
+val |= byte << offset;
+*dest = val;
+}
+
+/*
+ * calculating fan speed
+ * f_TOSC/4 is the clock, 8192Hz
+ * NP = tachometer pulses per revolution (usually 2)
+ * SR = number of periods(pulses) over which the clock ticks are counted
+ * TACH Count = SR x 8192 x 60 / (NP x RPM)
+ * RPM = SR x 8192 x 60 / (NP x TACH count)
+ *
+ * RPM mode - desired tach count is written to TACH Target Count
+ * PWM mode - desired duty cycle is written to PWMOUT Target Duty reg
+ */
+static void max31790_calculate_tach_count(MAX31790State *ms, uint8_t id)
+{
+uint32_t rpm;
+uint32_t sr = max31790_get_sr(ms->fan_dynamics[id]);
+ms->pwm_duty_cycle[id] = ms->pwmout[id] >> 7;
+rpm = (ms->max_rpm[id] * ms->pwm_duty_cycle[id]) / 0x1FF;
+
+if (rpm) {
+ms->tach_count[id] = (sr * MAX31790_CLK_FREQ * 60) /
+ (MAX31790_PULSES_PER_REV * rpm);
+} else {
+ms->tach_count[id] = 0;
+}
+
+}
+
+static void max31790_update_tach_count(MAX31790State *ms)
+{
+for (int i = 0; i < MAX31790_NUM_FANS; i++) {
+if (ms->fan_config[i] &
+(MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_TACH_INPUT_EN)) {
+ms->tach_count[i] = ms->target_count[i] >> 5;
+} else { /* PWM mode */
+max31790_calculate_tach_count(ms, i);
+}
+}
+}
+
+/* consecutive reads can increment the address up to 0xFF then wrap to 0 */
+/* slave to master */
+static uint8_t max31790_recv(I2CSlave *i2c)
+{
+MAX31790State *ms = MAX31790(i2c);
+uint8_t data, index, rem;
+
+max31790_update_tach_count(ms);
+
+if (ms->cmd_is_new) {
+ms->cmd_is_new = false;
+} else {
+ms->command++;
+}
+
+switch (ms->command) {
+case MAX31790_REG_GLOBAL_CONFIG:
+data = ms->global_config;
+break;
+
+case MAX31790_REG_PWM_FREQ:
+data = ms->pwm_freq;
+break;
+
+case MAX31790_REG_FAN_CONFIG(0) ...
+ MAX31790_REG_FAN_CONFIG(MAX31790_NUM_FANS - 1):
+data = ms->fan_config[ms->command - MAX31790_REG_FAN_CONFIG(0)];
+break;
+
+case MAX31790_REG_FAN_DYNAMICS(0) ...
+
