Re: [PATCH v1 2/4] hwmon: (sfctemp) Add StarFive JH71x0 temperature sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/8/23 04:40, Hal Feng wrote:
On Mon, 6 Feb 2023 11:21:38 -0800, Guenter Roeck wrote:
On 2/6/23 09:12, Hal Feng wrote:
On Tue, 3 Jan 2023 14:10:17 -0800, Guenter Roeck wrote:
On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote:
[...]
diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c
new file mode 100644
index 000000000000..e56716ad9587
--- /dev/null
+++ b/drivers/hwmon/sfctemp.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Emil Renner Berthing <kernel@xxxxxxxx>
+ * Copyright (C) 2021 Samin Guo <samin.guo@xxxxxxxxxxxxxxxx>
+ */
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/hwmon.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+/*
+ * TempSensor reset. The RSTN can be de-asserted once the analog core has
+ * powered up. Trst(min 100ns)
+ * 0:reset  1:de-assert
+ */
+#define SFCTEMP_RSTN    BIT(0)

Missing include of linux/bits.h

Will add it. Thanks.


+
+/*
+ * TempSensor analog core power down. The analog core will be powered up
+ * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the
+ * analog core is powered up.
+ * 0:power up  1:power down
+ */
+#define SFCTEMP_PD    BIT(1)
+
+/*
+ * TempSensor start conversion enable.
+ * 0:disable  1:enable
+ */
+#define SFCTEMP_RUN    BIT(2)
+
+/*
+ * TempSensor conversion value output.
+ * Temp(C)=DOUT*Y/4094 - K
+ */
+#define SFCTEMP_DOUT_POS    16
+#define SFCTEMP_DOUT_MSK    GENMASK(27, 16)
+
+/* DOUT to Celcius conversion constants */
+#define SFCTEMP_Y1000    237500L
+#define SFCTEMP_Z    4094L
+#define SFCTEMP_K1000    81100L
+
+struct sfctemp {
+    /* serialize access to hardware register and enabled below */
+    struct mutex lock;
+    struct completion conversion_done;
+    void __iomem *regs;
+    struct clk *clk_sense;
+    struct clk *clk_bus;
+    struct reset_control *rst_sense;
+    struct reset_control *rst_bus;
+    bool enabled;
+};
+
+static irqreturn_t sfctemp_isr(int irq, void *data)
+{
+    struct sfctemp *sfctemp = data;
+
+    complete(&sfctemp->conversion_done);
+    return IRQ_HANDLED;
+}
+
+static void sfctemp_power_up(struct sfctemp *sfctemp)
+{
+    /* make sure we're powered down first */
+    writel(SFCTEMP_PD, sfctemp->regs);
+    udelay(1);
+
+    writel(0, sfctemp->regs);
+    /* wait t_pu(50us) + t_rst(100ns) */
+    usleep_range(60, 200);
+
+    /* de-assert reset */
+    writel(SFCTEMP_RSTN, sfctemp->regs);
+    udelay(1); /* wait t_su(500ps) */
+}
+
+static void sfctemp_power_down(struct sfctemp *sfctemp)
+{
+    writel(SFCTEMP_PD, sfctemp->regs);
+}
+
+static void sfctemp_run_single(struct sfctemp *sfctemp)
+{
+    writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs);
+    udelay(1);
+    writel(SFCTEMP_RSTN, sfctemp->regs);

The datasheet (or, rather, programming manual) does not appear
to be public, so I have to guess here.

The code suggests that running a single conversion may be a choice,
not a requirement. If it is indeed a choice, the reasoning needs to be
explained since it adds a lot of complexity and dependencies to the
driver (for example, interrupt support is only mandatory or even needed
due to this choice). It also adds a significant delay to temperature
read operations, which may have practical impact on thermal control
software.

If the chip only supports single temperature readings, that needs to be
explained as well (and why SFCTEMP_RUN has to be reset in that case).

The chip supports continuous conversion. When you set SFCTEMP_RUN, the
temperature raw data will be generated all the time. However, it will
also generate interrupts all the time when the conversion is finished,
because of the hardware limitation. So in this driver, we just support
the single conversion.


Sorry, I don't follow the logic. The interrupt is, for all practical
purposes, useless because there are no limits and exceeding any such
limits is therefore not supported. The only reason to have and enable
to interrupt is because continuous mode is disabled.

The code could be simplified a lot if interrupt support would be
dropped and continuous mode would be enabled.

If we enable continuous mode, which means SFCTEMP_RUN remains asserted,
the conversion finished interrupt will be raised after each sample
time (8.192 ms). Within a few minutes, a lot of interrupts are raised,
as showed below.

# cat /proc/interrupts
            CPU0       CPU1       CPU2       CPU3
   1:          0          0          0          0  SiFive PLIC   1 Edge      ccache_ecc
   2:          1          0          0          0  SiFive PLIC   3 Edge      ccache_ecc
   3:          1          0          0          0  SiFive PLIC   4 Edge      ccache_ecc
   4:          0          0          0          0  SiFive PLIC   2 Edge      ccache_ecc
   5:       1116       1670        411       1466  RISC-V INTC   5 Edge      riscv-timer
   6:      32093          0          0          0  SiFive PLIC  81 Edge      120e0000.temperature-sensor
  10:       1233          0          0          0  SiFive PLIC  32 Edge      ttyS0
IPI0:       117         62        123        117  Rescheduling interrupts
IPI1:       278        353        105        273  Function call interrupts
IPI2:         0          0          0          0  CPU stop interrupts
IPI3:         0          0          0          0  CPU stop (for crash dump) interrupts
IPI4:         0          0          0          0  IRQ work interrupts
IPI5:         0          0          0          0  Timer broadcast interrupts

If we enable continuous mode and drop the interrupt support in the
driver, the kernel will not know the interrupts but a lot of interrupts
are still raised in hardware. Can we do such like that?

Why not ? It just stays raised. That happens a lot.

Without the interrupt support, the temperature we read may be the value
generated in the last cycle.

That would be highly unusual and should be documented.



I think the temperature has its value only when we read it, so we start

"may be" ? "I think" ? That means you don't know ? Maybe test it, or ask
the chip designers.

conversion only when we read the temperature. Further more, it will
consume more power if we enable continuous mode.


Usually that is not a concern, much less so than delaying each reader.

Ultimately, sure, you can do whatever you want. I'll still accept the driver.
I do expect you to explain your reasons (all of them) in the driver, though.

If you don't _know_ if the temperature is updated in continuous mode,
please state exactly that in the comments. Also explain how much power
is saved by not running in continuous mode. I don't want anyone to come
back later on and change the code because they don't know the reasons
why it doesn't use continuous mode.

Thanks,
Guenter

Best regards,
Hal




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux