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

[Guenter Roeck <linux@roeck-us.net>]

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