Messages in this thread | | | Date | Wed, 8 Feb 2023 07:16:52 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCH v1 2/4] hwmon: (sfctemp) Add StarFive JH71x0 temperature sensor |
| |
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@esmil.dk> >>>>> + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> >>>>> + */ >>>>> +#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
| |