[PATCH v1 0/4] Temperature sensor support for StarFive JH7110 RISC-V SoC

[PATCH v1 0/4] Temperature sensor support for StarFive JH7110 RISC-V SoC

[Hal Feng]

This patch series adds temperature sensor support for StarFive JH7110 SoC.
The last two patches depend on series [1].

[1]: https://lore.kernel.org/all/20221220011247.35560-1-hal.feng@starfivetech.com/

Emil Renner Berthing (4):
  dt-bindings: hwmon: Add starfive,jh71x0-temp
  hwmon: (sfctemp) Add StarFive JH71x0 temperature sensor
  riscv: dts: starfive: jh7110: Add temperature sensor node
  riscv: dts: starfive: visionfive-2: Add thermal-zones

 .../bindings/hwmon/starfive,jh71x0-temp.yaml  |  75 ++++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/sfctemp.rst               |  33 ++
 MAINTAINERS                                   |   8 +
 .../jh7110-starfive-visionfive-2.dtsi         |  28 ++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  13 +
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/sfctemp.c                       | 350 ++++++++++++++++++
 9 files changed, 519 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
 create mode 100644 Documentation/hwmon/sfctemp.rst
 create mode 100644 drivers/hwmon/sfctemp.c


base-commit: 830b3c68c1fb1e9176028d02ef86f3cf76aa2476
prerequisite-patch-id: 4dc515731ce237184553c1606ffb3afaeb51c3d8
prerequisite-patch-id: 09c98554df52d17ba5fd604125f8cdd62cbe80d1
prerequisite-patch-id: a798370d170dc2bcc79ed86f741c21c1e6d87c78
prerequisite-patch-id: bd9fd8b5cb2376dc7a5e08e1a1fbb969cf475926
prerequisite-patch-id: c57ebb83bc43ccd2a8366ff166eb499da1e1d2cf
prerequisite-patch-id: a1673a9e9f19d6fab5a51abb721e54e36636f067
prerequisite-patch-id: 94860423c7acc9025249d4bb36652a585bd0a797
prerequisite-patch-id: b5084253283929d9a6d0e66c350400c7c85d034d
prerequisite-patch-id: 6e369dbe9dca2785e4ea7d0b80e525e227a90a6e
prerequisite-patch-id: e08806183c152714c563f3a21c6d7b2f539c4d6e
prerequisite-patch-id: 79db8036abdc48fd36da227652ec62627a6b548b
prerequisite-patch-id: 06971b8e6bddc0e87e63bfdb0ce8bfb653bd73aa
prerequisite-patch-id: 16309a0e23811a2c55d2e56886de3e8eccc51554
prerequisite-patch-id: bf4f7ab0b6cfa90b6e49e66c7d75ed2eaaebbe78
prerequisite-patch-id: 38468d532e87867990055d3320679f18c5f52278
prerequisite-patch-id: 4710f2ac22dca0bdd9ff5d744d2c37cab3c74515
prerequisite-patch-id: 6bb9a780c62af3bcc2368dfd20303c7b1bc91e23
prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
prerequisite-patch-id: e3b986b9c60b2b93b7812ec174c9e1b4cfb14c97
prerequisite-patch-id: a2b3a9cff8a683422eb0ccf3a0850091401812d4
prerequisite-patch-id: e0ba7af0f8d3d41844da9fbcba14b548cbc18f55
prerequisite-patch-id: bc0176325c11a632c6abaa83e54e891cc92d1c74
-- 
2.38.1

[PATCH v1 1/4] dt-bindings: hwmon: Add starfive,jh71x0-temp

[Hal Feng]

From: Emil Renner Berthing <kernel@esmil.dk>

Add bindings for the temperature sensor on the StarFive JH7100 and
JH7110 SoCs.

Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../bindings/hwmon/starfive,jh71x0-temp.yaml  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml b/Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
new file mode 100644
index 000000000000..2600881e2cdc
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/starfive,jh71x0-temp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH71x0 Temperature Sensor
+
+maintainers:
+  - Emil Renner Berthing <kernel@esmil.dk>
+
+description: |
+  StarFive Technology Co. JH71x0 embedded temperature sensor
+
+properties:
+  compatible:
+    enum:
+      - starfive,jh7100-temp
+      - starfive,jh7110-temp
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: "sense"
+      - const: "bus"
+
+  '#thermal-sensor-cells':
+    const: 0
+
+  interrupts:
+    maxItems: 1
+
+  resets:
+    minItems: 2
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: "sense"
+      - const: "bus"
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/starfive-jh7100.h>
+    #include <dt-bindings/reset/starfive-jh7100.h>
+
+    tmon@124a0000 {
+        compatible = "starfive,jh7100-temp";
+        reg = <0x124a0000 0x10000>;
+        clocks = <&clkgen JH7100_CLK_TEMP_SENSE>,
+                 <&clkgen JH7100_CLK_TEMP_APB>;
+        clock-names = "sense", "bus";
+        #thermal-sensor-cells = <0>;
+        interrupts = <122>;
+        resets = <&rstgen JH7100_RSTN_TEMP_SENSE>,
+                 <&rstgen JH7100_RSTN_TEMP_APB>;
+        reset-names = "sense", "bus";
+    };
-- 
2.38.1

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

[Hal Feng]

From: Emil Renner Berthing <kernel@esmil.dk>

Register definitions and conversion constants based on sfctemp driver by
Samin in the StarFive 5.10 kernel.

Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
Co-developed-by: Samin Guo <samin.guo@starfivetech.com>
Signed-off-by: Samin Guo <samin.guo@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/sfctemp.rst |  33 +++
 MAINTAINERS                     |   8 +
 drivers/hwmon/Kconfig           |  10 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/sfctemp.c         | 350 ++++++++++++++++++++++++++++++++
 6 files changed, 403 insertions(+)
 create mode 100644 Documentation/hwmon/sfctemp.rst
 create mode 100644 drivers/hwmon/sfctemp.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index c1d11cf13eef..f7ede608b6e3 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers
    sch5627
    sch5636
    scpi-hwmon
+   sfctemp
    sht15
    sht21
    sht3x
diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst
new file mode 100644
index 000000000000..9fbd5bb1f356
--- /dev/null
+++ b/Documentation/hwmon/sfctemp.rst
@@ -0,0 +1,33 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver sfctemp
+=====================
+
+Supported chips:
+ - StarFive JH7100
+ - StarFive JH7110
+
+Authors:
+ - Emil Renner Berthing <kernel@esmil.dk>
+
+Description
+-----------
+
+This driver adds support for reading the built-in temperature sensor on the
+JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd.
+
+``sysfs`` interface
+-------------------
+
+The temperature sensor can be enabled, disabled and queried via the standard
+hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of
+``X``:
+
+================ ==== =============================================
+Name             Perm Description
+================ ==== =============================================
+temp1_enable     RW   Enable or disable temperature sensor.
+                      Automatically enabled by the driver,
+                      but may be disabled to save power.
+temp1_input      RO   Temperature reading in milli-degrees Celsius.
+================ ==== =============================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 85e8f83161d7..ab3cd5827b26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18661,6 +18661,14 @@ L:	netdev@vger.kernel.org
 S:	Supported
 F:	drivers/net/ethernet/sfc/
 
+SFCTEMP HWMON DRIVER
+M:	Emil Renner Berthing <kernel@esmil.dk>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
+F:	Documentation/hwmon/sfctemp.rst
+F:	drivers/hwmon/sfctemp.c
+
 SFF/SFP/SFP+ MODULE SUPPORT
 M:	Russell King <linux@armlinux.org.uk>
 L:	netdev@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ac3daaf59ce..c6bbfcca3a14 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1910,6 +1910,16 @@ config SENSORS_STTS751
 	  This driver can also be built as a module. If so, the module
 	  will be called stts751.
 
+config SENSORS_SFCTEMP
+	tristate "Starfive JH71x0 temperature sensor"
+	depends on SOC_STARFIVE || COMPILE_TEST
+	help
+	  If you say yes here you get support for temperature sensor
+	  on the Starfive JH71x0 SoCs.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sfctemp.
+
 config SENSORS_SMM665
 	tristate "Summit Microelectronics SMM665"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 11d076cad8a2..5a4a02c5535c 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI)	+= sbrmi.o
 obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
 obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
 obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
+obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
 obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
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)
+
+/*
+ * 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);
+}
+
+static int sfctemp_enable(struct sfctemp *sfctemp)
+{
+	int ret = 0;
+
+	mutex_lock(&sfctemp->lock);
+	if (sfctemp->enabled)
+		goto done;
+
+	ret = clk_prepare_enable(sfctemp->clk_bus);
+	if (ret)
+		goto err;
+	ret = reset_control_deassert(sfctemp->rst_bus);
+	if (ret)
+		goto err_disable_bus;
+
+	ret = clk_prepare_enable(sfctemp->clk_sense);
+	if (ret)
+		goto err_assert_bus;
+	ret = reset_control_deassert(sfctemp->rst_sense);
+	if (ret)
+		goto err_disable_sense;
+
+	sfctemp_power_up(sfctemp);
+	sfctemp->enabled = true;
+done:
+	mutex_unlock(&sfctemp->lock);
+	return ret;
+
+err_disable_sense:
+	clk_disable_unprepare(sfctemp->clk_sense);
+err_assert_bus:
+	reset_control_assert(sfctemp->rst_bus);
+err_disable_bus:
+	clk_disable_unprepare(sfctemp->clk_bus);
+err:
+	mutex_unlock(&sfctemp->lock);
+	return ret;
+}
+
+static int sfctemp_disable(struct sfctemp *sfctemp)
+{
+	mutex_lock(&sfctemp->lock);
+	if (!sfctemp->enabled)
+		goto done;
+
+	sfctemp_power_down(sfctemp);
+	reset_control_assert(sfctemp->rst_sense);
+	clk_disable_unprepare(sfctemp->clk_sense);
+	reset_control_assert(sfctemp->rst_bus);
+	clk_disable_unprepare(sfctemp->clk_bus);
+	sfctemp->enabled = false;
+done:
+	mutex_unlock(&sfctemp->lock);
+	return 0;
+}
+
+static void sfctemp_disable_action(void *data)
+{
+	sfctemp_disable(data);
+}
+
+static int sfctemp_convert(struct sfctemp *sfctemp, long *val)
+{
+	int ret;
+
+	mutex_lock(&sfctemp->lock);
+	if (!sfctemp->enabled) {
+		ret = -ENODATA;
+		goto out;
+	}
+
+	sfctemp_run_single(sfctemp);
+
+	ret = wait_for_completion_interruptible_timeout(&sfctemp->conversion_done,
+							msecs_to_jiffies(10));
+	if (ret <= 0) {
+		if (ret == 0)
+			ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	/* calculate temperature in milli Celcius */
+	*val = (long)((readl(sfctemp->regs) & SFCTEMP_DOUT_MSK) >> SFCTEMP_DOUT_POS)
+		* SFCTEMP_Y1000 / SFCTEMP_Z - SFCTEMP_K1000;
+
+	ret = 0;
+out:
+	mutex_unlock(&sfctemp->lock);
+	return ret;
+}
+
+static umode_t sfctemp_is_visible(const void *data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_enable:
+			return 0644;
+		case hwmon_temp_input:
+			return 0444;
+		}
+		return 0;
+	default:
+		return 0;
+	}
+}
+
+static int sfctemp_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct sfctemp *sfctemp = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_enable:
+			*val = sfctemp->enabled;
+			return 0;
+		case hwmon_temp_input:
+			return sfctemp_convert(sfctemp, val);
+		}
+		return -EINVAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sfctemp_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	struct sfctemp *sfctemp = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_enable:
+			if (val == 0)
+				return sfctemp_disable(sfctemp);
+			if (val == 1)
+				return sfctemp_enable(sfctemp);
+			break;
+		}
+		return -EINVAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct hwmon_channel_info *sfctemp_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_ENABLE | HWMON_T_INPUT),
+	NULL
+};
+
+static const struct hwmon_ops sfctemp_hwmon_ops = {
+	.is_visible = sfctemp_is_visible,
+	.read = sfctemp_read,
+	.write = sfctemp_write,
+};
+
+static const struct hwmon_chip_info sfctemp_chip_info = {
+	.ops = &sfctemp_hwmon_ops,
+	.info = sfctemp_info,
+};
+
+static int sfctemp_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device *hwmon_dev;
+	struct sfctemp *sfctemp;
+	int ret;
+
+	sfctemp = devm_kzalloc(dev, sizeof(*sfctemp), GFP_KERNEL);
+	if (!sfctemp)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, sfctemp);
+	mutex_init(&sfctemp->lock);
+	init_completion(&sfctemp->conversion_done);
+
+	sfctemp->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sfctemp->regs))
+		return PTR_ERR(sfctemp->regs);
+
+	sfctemp->clk_sense = devm_clk_get(dev, "sense");
+	if (IS_ERR(sfctemp->clk_sense))
+		return dev_err_probe(dev, PTR_ERR(sfctemp->clk_sense),
+				     "error getting sense clock\n");
+
+	sfctemp->clk_bus = devm_clk_get(dev, "bus");
+	if (IS_ERR(sfctemp->clk_bus))
+		return dev_err_probe(dev, PTR_ERR(sfctemp->clk_bus),
+				     "error getting bus clock\n");
+
+	sfctemp->rst_sense = devm_reset_control_get_exclusive(dev, "sense");
+	if (IS_ERR(sfctemp->rst_sense))
+		return dev_err_probe(dev, PTR_ERR(sfctemp->rst_sense),
+				     "error getting sense reset\n");
+
+	sfctemp->rst_bus = devm_reset_control_get_exclusive(dev, "bus");
+	if (IS_ERR(sfctemp->rst_bus))
+		return dev_err_probe(dev, PTR_ERR(sfctemp->rst_bus),
+				     "error getting busreset\n");
+
+	ret = reset_control_assert(sfctemp->rst_sense);
+	if (ret)
+		return dev_err_probe(dev, ret, "error asserting sense reset\n");
+
+	ret = reset_control_assert(sfctemp->rst_bus);
+	if (ret)
+		return dev_err_probe(dev, ret, "error asserting bus reset\n");
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_request_irq(dev, ret, sfctemp_isr, 0, pdev->name, sfctemp);
+	if (ret)
+		return dev_err_probe(dev, ret, "error requesting irq\n");
+
+	ret = devm_add_action(dev, sfctemp_disable_action, sfctemp);
+	if (ret)
+		return ret;
+
+	ret = sfctemp_enable(sfctemp);
+	if (ret)
+		return dev_err_probe(dev, ret, "error enabling temperature sensor: %d\n", ret);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, pdev->name, sfctemp,
+							 &sfctemp_chip_info, NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id sfctemp_of_match[] = {
+	{ .compatible = "starfive,jh7100-temp" },
+	{ .compatible = "starfive,jh7110-temp" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sfctemp_of_match);
+
+static struct platform_driver sfctemp_driver = {
+	.probe  = sfctemp_probe,
+	.driver = {
+		.name = "sfctemp",
+		.of_match_table = sfctemp_of_match,
+	},
+};
+module_platform_driver(sfctemp_driver);
+
+MODULE_AUTHOR("Emil Renner Berthing");
+MODULE_DESCRIPTION("StarFive JH71x0 temperature sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.38.1

[PATCH v1 3/4] riscv: dts: starfive: jh7110: Add temperature sensor node

[Hal Feng]

From: Emil Renner Berthing <kernel@esmil.dk>

Add temperature sensor support for StarFive JH7110 SoC.

Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 64d260ea1f29..793ae26bf72d 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -353,6 +353,19 @@ uart5: serial@12020000 {
 			status = "disabled";
 		};
 
+		sfctemp: tmon@120e0000  {
+			compatible = "starfive,jh7110-temp";
+			reg = <0x0 0x120e0000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_TEMP_CORE>,
+				 <&syscrg JH7110_SYSCLK_TEMP_APB>;
+			clock-names = "sense", "bus";
+			resets = <&syscrg JH7110_SYSRST_TEMP_CORE>,
+				 <&syscrg JH7110_SYSRST_TEMP_APB>;
+			reset-names = "sense", "bus";
+			interrupts = <81>;
+			#thermal-sensor-cells = <0>;
+		};
+
 		syscrg: clock-controller@13020000 {
 			compatible = "starfive,jh7110-syscrg";
 			reg = <0x0 0x13020000 0x0 0x10000>;
-- 
2.38.1

[PATCH v1 4/4] riscv: dts: starfive: visionfive-2: Add thermal-zones

[Hal Feng]

From: Emil Renner Berthing <kernel@esmil.dk>

Add thermal-zones for StarFive VisionFive 2 board.

Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../jh7110-starfive-visionfive-2.dtsi         | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index c60280b89c73..c7c7735f021d 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -27,6 +27,34 @@ memory@40000000 {
 		reg = <0x0 0x40000000 0x1 0x0>;
 	};
 
+	thermal-zones {
+		cpu-thermal {
+			polling-delay-passive = <250>;
+			polling-delay = <15000>;
+
+			thermal-sensors = <&sfctemp>;
+
+			cooling-maps {
+			};
+
+			trips {
+				cpu_alert0: cpu_alert0 {
+					/* milliCelsius */
+					temperature = <75000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_crit: cpu_crit {
+					/* milliCelsius */
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+	};
+
 	gpio-restart {
 		compatible = "gpio-restart";
 		gpios = <&gpio 35 GPIO_ACTIVE_HIGH>;
-- 
2.38.1

Re: [PATCH v1 0/4] Temperature sensor support for StarFive JH7110 RISC-V SoC

[Guenter Roeck]

On Tue, Jan 03, 2023 at 09:31:41AM +0800, Hal Feng wrote:
> This patch series adds temperature sensor support for StarFive JH7110 SoC.
> The last two patches depend on series [1].
> 
> [1]: https://lore.kernel.org/all/20221220011247.35560-1-hal.feng@starfivetech.com/
> 
> Emil Renner Berthing (4):
>   dt-bindings: hwmon: Add starfive,jh71x0-temp
>   hwmon: (sfctemp) Add StarFive JH71x0 temperature sensor
>   riscv: dts: starfive: jh7110: Add temperature sensor node
>   riscv: dts: starfive: visionfive-2: Add thermal-zones
> 

The hardware monitoring driver is obviously either the same
or derived from the previous series at
https://patchwork.kernel.org/project/linux-hwmon/list/?series=&submitter=&state=*&q=starfive

Why is this not submitted as v4 of the original series ?
What has changed, and what is the rationale for (re-)submitting
it as v1 ?

Guenter

>  .../bindings/hwmon/starfive,jh71x0-temp.yaml  |  75 ++++
>  Documentation/hwmon/index.rst                 |   1 +
>  Documentation/hwmon/sfctemp.rst               |  33 ++
>  MAINTAINERS                                   |   8 +
>  .../jh7110-starfive-visionfive-2.dtsi         |  28 ++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      |  13 +
>  drivers/hwmon/Kconfig                         |  10 +
>  drivers/hwmon/Makefile                        |   1 +
>  drivers/hwmon/sfctemp.c                       | 350 ++++++++++++++++++
>  9 files changed, 519 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
>  create mode 100644 Documentation/hwmon/sfctemp.rst
>  create mode 100644 drivers/hwmon/sfctemp.c
> 
> 
> base-commit: 830b3c68c1fb1e9176028d02ef86f3cf76aa2476
> prerequisite-patch-id: 4dc515731ce237184553c1606ffb3afaeb51c3d8
> prerequisite-patch-id: 09c98554df52d17ba5fd604125f8cdd62cbe80d1
> prerequisite-patch-id: a798370d170dc2bcc79ed86f741c21c1e6d87c78
> prerequisite-patch-id: bd9fd8b5cb2376dc7a5e08e1a1fbb969cf475926
> prerequisite-patch-id: c57ebb83bc43ccd2a8366ff166eb499da1e1d2cf
> prerequisite-patch-id: a1673a9e9f19d6fab5a51abb721e54e36636f067
> prerequisite-patch-id: 94860423c7acc9025249d4bb36652a585bd0a797
> prerequisite-patch-id: b5084253283929d9a6d0e66c350400c7c85d034d
> prerequisite-patch-id: 6e369dbe9dca2785e4ea7d0b80e525e227a90a6e
> prerequisite-patch-id: e08806183c152714c563f3a21c6d7b2f539c4d6e
> prerequisite-patch-id: 79db8036abdc48fd36da227652ec62627a6b548b
> prerequisite-patch-id: 06971b8e6bddc0e87e63bfdb0ce8bfb653bd73aa
> prerequisite-patch-id: 16309a0e23811a2c55d2e56886de3e8eccc51554
> prerequisite-patch-id: bf4f7ab0b6cfa90b6e49e66c7d75ed2eaaebbe78
> prerequisite-patch-id: 38468d532e87867990055d3320679f18c5f52278
> prerequisite-patch-id: 4710f2ac22dca0bdd9ff5d744d2c37cab3c74515
> prerequisite-patch-id: 6bb9a780c62af3bcc2368dfd20303c7b1bc91e23
> prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
> prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
> prerequisite-patch-id: e3b986b9c60b2b93b7812ec174c9e1b4cfb14c97
> prerequisite-patch-id: a2b3a9cff8a683422eb0ccf3a0850091401812d4
> prerequisite-patch-id: e0ba7af0f8d3d41844da9fbcba14b548cbc18f55
> prerequisite-patch-id: bc0176325c11a632c6abaa83e54e891cc92d1c74
> -- 
> 2.38.1
> 

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

[Guenter Roeck]

On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> Register definitions and conversion constants based on sfctemp driver by
> Samin in the StarFive 5.10 kernel.
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> Co-developed-by: Samin Guo <samin.guo@starfivetech.com>
> Signed-off-by: Samin Guo <samin.guo@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>

This is obviously version 4 of the previous patch series,
with various enhancements. Please do not "sell" that as
v1 of a new patch series. Submit it as next version,
and provide a change log.

> ---
>  Documentation/hwmon/index.rst   |   1 +
>  Documentation/hwmon/sfctemp.rst |  33 +++
>  MAINTAINERS                     |   8 +
>  drivers/hwmon/Kconfig           |  10 +
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/sfctemp.c         | 350 ++++++++++++++++++++++++++++++++
>  6 files changed, 403 insertions(+)
>  create mode 100644 Documentation/hwmon/sfctemp.rst
>  create mode 100644 drivers/hwmon/sfctemp.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index c1d11cf13eef..f7ede608b6e3 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers
>     sch5627
>     sch5636
>     scpi-hwmon
> +   sfctemp
>     sht15
>     sht21
>     sht3x
> diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst
> new file mode 100644
> index 000000000000..9fbd5bb1f356
> --- /dev/null
> +++ b/Documentation/hwmon/sfctemp.rst
> @@ -0,0 +1,33 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver sfctemp
> +=====================
> +
> +Supported chips:
> + - StarFive JH7100
> + - StarFive JH7110
> +
> +Authors:
> + - Emil Renner Berthing <kernel@esmil.dk>
> +
> +Description
> +-----------
> +
> +This driver adds support for reading the built-in temperature sensor on the
> +JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd.
> +
> +``sysfs`` interface
> +-------------------
> +
> +The temperature sensor can be enabled, disabled and queried via the standard
> +hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of
> +``X``:
> +
> +================ ==== =============================================
> +Name             Perm Description
> +================ ==== =============================================
> +temp1_enable     RW   Enable or disable temperature sensor.
> +                      Automatically enabled by the driver,
> +                      but may be disabled to save power.
> +temp1_input      RO   Temperature reading in milli-degrees Celsius.
> +================ ==== =============================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 85e8f83161d7..ab3cd5827b26 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18661,6 +18661,14 @@ L:	netdev@vger.kernel.org
>  S:	Supported
>  F:	drivers/net/ethernet/sfc/
>  
> +SFCTEMP HWMON DRIVER
> +M:	Emil Renner Berthing <kernel@esmil.dk>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
> +F:	Documentation/hwmon/sfctemp.rst
> +F:	drivers/hwmon/sfctemp.c
> +
>  SFF/SFP/SFP+ MODULE SUPPORT
>  M:	Russell King <linux@armlinux.org.uk>
>  L:	netdev@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7ac3daaf59ce..c6bbfcca3a14 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1910,6 +1910,16 @@ config SENSORS_STTS751
>  	  This driver can also be built as a module. If so, the module
>  	  will be called stts751.
>  
> +config SENSORS_SFCTEMP
> +	tristate "Starfive JH71x0 temperature sensor"
> +	depends on SOC_STARFIVE || COMPILE_TEST
> +	help
> +	  If you say yes here you get support for temperature sensor
> +	  on the Starfive JH71x0 SoCs.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called sfctemp.
> +
>  config SENSORS_SMM665
>  	tristate "Summit Microelectronics SMM665"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 11d076cad8a2..5a4a02c5535c 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI)	+= sbrmi.o
>  obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>  obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>  obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
> +obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
>  obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>  obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> 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

> +
> +/*
> + * 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).

> +}
> +
> +static int sfctemp_enable(struct sfctemp *sfctemp)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&sfctemp->lock);
> +	if (sfctemp->enabled)
> +		goto done;
> +
> +	ret = clk_prepare_enable(sfctemp->clk_bus);
> +	if (ret)
> +		goto err;
> +	ret = reset_control_deassert(sfctemp->rst_bus);
> +	if (ret)
> +		goto err_disable_bus;
> +
> +	ret = clk_prepare_enable(sfctemp->clk_sense);
> +	if (ret)
> +		goto err_assert_bus;
> +	ret = reset_control_deassert(sfctemp->rst_sense);
> +	if (ret)
> +		goto err_disable_sense;
> +
> +	sfctemp_power_up(sfctemp);
> +	sfctemp->enabled = true;
> +done:
> +	mutex_unlock(&sfctemp->lock);
> +	return ret;
> +
> +err_disable_sense:
> +	clk_disable_unprepare(sfctemp->clk_sense);
> +err_assert_bus:
> +	reset_control_assert(sfctemp->rst_bus);
> +err_disable_bus:
> +	clk_disable_unprepare(sfctemp->clk_bus);
> +err:
> +	mutex_unlock(&sfctemp->lock);
> +	return ret;
> +}
> +
> +static int sfctemp_disable(struct sfctemp *sfctemp)
> +{
> +	mutex_lock(&sfctemp->lock);
> +	if (!sfctemp->enabled)
> +		goto done;
> +
> +	sfctemp_power_down(sfctemp);
> +	reset_control_assert(sfctemp->rst_sense);
> +	clk_disable_unprepare(sfctemp->clk_sense);
> +	reset_control_assert(sfctemp->rst_bus);
> +	clk_disable_unprepare(sfctemp->clk_bus);
> +	sfctemp->enabled = false;
> +done:
> +	mutex_unlock(&sfctemp->lock);
> +	return 0;
> +}
> +
> +static void sfctemp_disable_action(void *data)
> +{
> +	sfctemp_disable(data);
> +}
> +
> +static int sfctemp_convert(struct sfctemp *sfctemp, long *val)
> +{
> +	int ret;
> +
> +	mutex_lock(&sfctemp->lock);
> +	if (!sfctemp->enabled) {
> +		ret = -ENODATA;
> +		goto out;
> +	}
> +
> +	sfctemp_run_single(sfctemp);
> +
> +	ret = wait_for_completion_interruptible_timeout(&sfctemp->conversion_done,
> +							msecs_to_jiffies(10));
> +	if (ret <= 0) {
> +		if (ret == 0)
> +			ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	/* calculate temperature in milli Celcius */
> +	*val = (long)((readl(sfctemp->regs) & SFCTEMP_DOUT_MSK) >> SFCTEMP_DOUT_POS)
> +		* SFCTEMP_Y1000 / SFCTEMP_Z - SFCTEMP_K1000;
> +
> +	ret = 0;
> +out:
> +	mutex_unlock(&sfctemp->lock);
> +	return ret;
> +}
> +
> +static umode_t sfctemp_is_visible(const void *data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_enable:
> +			return 0644;
> +		case hwmon_temp_input:
> +			return 0444;
> +		}
> +		return 0;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int sfctemp_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct sfctemp *sfctemp = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_enable:
> +			*val = sfctemp->enabled;
> +			return 0;
> +		case hwmon_temp_input:
> +			return sfctemp_convert(sfctemp, val);
> +		}
> +		return -EINVAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int sfctemp_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	struct sfctemp *sfctemp = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_enable:
> +			if (val == 0)
> +				return sfctemp_disable(sfctemp);
> +			if (val == 1)
> +				return sfctemp_enable(sfctemp);
> +			break;
> +		}
> +		return -EINVAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct hwmon_channel_info *sfctemp_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_ENABLE | HWMON_T_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops sfctemp_hwmon_ops = {
> +	.is_visible = sfctemp_is_visible,
> +	.read = sfctemp_read,
> +	.write = sfctemp_write,
> +};
> +
> +static const struct hwmon_chip_info sfctemp_chip_info = {
> +	.ops = &sfctemp_hwmon_ops,
> +	.info = sfctemp_info,
> +};
> +
> +static int sfctemp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device *hwmon_dev;
> +	struct sfctemp *sfctemp;
> +	int ret;
> +
> +	sfctemp = devm_kzalloc(dev, sizeof(*sfctemp), GFP_KERNEL);
> +	if (!sfctemp)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, sfctemp);
> +	mutex_init(&sfctemp->lock);
> +	init_completion(&sfctemp->conversion_done);
> +
> +	sfctemp->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(sfctemp->regs))
> +		return PTR_ERR(sfctemp->regs);
> +
> +	sfctemp->clk_sense = devm_clk_get(dev, "sense");
> +	if (IS_ERR(sfctemp->clk_sense))
> +		return dev_err_probe(dev, PTR_ERR(sfctemp->clk_sense),
> +				     "error getting sense clock\n");
> +
> +	sfctemp->clk_bus = devm_clk_get(dev, "bus");
> +	if (IS_ERR(sfctemp->clk_bus))
> +		return dev_err_probe(dev, PTR_ERR(sfctemp->clk_bus),
> +				     "error getting bus clock\n");
> +
> +	sfctemp->rst_sense = devm_reset_control_get_exclusive(dev, "sense");
> +	if (IS_ERR(sfctemp->rst_sense))
> +		return dev_err_probe(dev, PTR_ERR(sfctemp->rst_sense),
> +				     "error getting sense reset\n");
> +
> +	sfctemp->rst_bus = devm_reset_control_get_exclusive(dev, "bus");
> +	if (IS_ERR(sfctemp->rst_bus))
> +		return dev_err_probe(dev, PTR_ERR(sfctemp->rst_bus),
> +				     "error getting busreset\n");
> +
> +	ret = reset_control_assert(sfctemp->rst_sense);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "error asserting sense reset\n");
> +
> +	ret = reset_control_assert(sfctemp->rst_bus);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "error asserting bus reset\n");
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_request_irq(dev, ret, sfctemp_isr, 0, pdev->name, sfctemp);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "error requesting irq\n");
> +
> +	ret = devm_add_action(dev, sfctemp_disable_action, sfctemp);
> +	if (ret)
> +		return ret;
> +
> +	ret = sfctemp_enable(sfctemp);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "error enabling temperature sensor: %d\n", ret);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, pdev->name, sfctemp,
> +							 &sfctemp_chip_info, NULL);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id sfctemp_of_match[] = {
> +	{ .compatible = "starfive,jh7100-temp" },
> +	{ .compatible = "starfive,jh7110-temp" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sfctemp_of_match);
> +
> +static struct platform_driver sfctemp_driver = {
> +	.probe  = sfctemp_probe,
> +	.driver = {
> +		.name = "sfctemp",
> +		.of_match_table = sfctemp_of_match,
> +	},
> +};
> +module_platform_driver(sfctemp_driver);
> +
> +MODULE_AUTHOR("Emil Renner Berthing");
> +MODULE_DESCRIPTION("StarFive JH71x0 temperature sensor driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v1 1/4] dt-bindings: hwmon: Add starfive,jh71x0-temp

[Guenter Roeck]

On Tue, Jan 03, 2023 at 09:31:42AM +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> Add bindings for the temperature sensor on the StarFive JH7100 and
> JH7110 SoCs.
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>

The previous version of this bindings document (v3, 
https://patchwork.kernel.org/project/linux-hwmon/patch/20210726171802.1052716-2-kernel@esmil.dk/
had been reviewed by Rob Herring. Even though this version is named differently
(starfive,jh71x0-temp.yaml instead of starfive,jh7100-temp.yaml), the old version
should be referenced, and there should be a change log.

Guenter

> ---
>  .../bindings/hwmon/starfive,jh71x0-temp.yaml  | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml b/Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
> new file mode 100644
> index 000000000000..2600881e2cdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/starfive,jh71x0-temp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH71x0 Temperature Sensor
> +
> +maintainers:
> +  - Emil Renner Berthing <kernel@esmil.dk>
> +
> +description: |
> +  StarFive Technology Co. JH71x0 embedded temperature sensor
> +
> +properties:
> +  compatible:
> +    enum:
> +      - starfive,jh7100-temp
> +      - starfive,jh7110-temp
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - const: "sense"
> +      - const: "bus"
> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  resets:
> +    minItems: 2
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: "sense"
> +      - const: "bus"
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - resets
> +  - reset-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/starfive-jh7100.h>
> +    #include <dt-bindings/reset/starfive-jh7100.h>
> +
> +    tmon@124a0000 {
> +        compatible = "starfive,jh7100-temp";
> +        reg = <0x124a0000 0x10000>;
> +        clocks = <&clkgen JH7100_CLK_TEMP_SENSE>,
> +                 <&clkgen JH7100_CLK_TEMP_APB>;
> +        clock-names = "sense", "bus";
> +        #thermal-sensor-cells = <0>;
> +        interrupts = <122>;
> +        resets = <&rstgen JH7100_RSTN_TEMP_SENSE>,
> +                 <&rstgen JH7100_RSTN_TEMP_APB>;
> +        reset-names = "sense", "bus";
> +    };

Re: [PATCH v1 0/4] Temperature sensor support for StarFive JH7110 RISC-V SoC

[Hal Feng]

On Mon, 2 Jan 2023 17:56:01 -0800, Guenter Roeck wrote:
> On Tue, Jan 03, 2023 at 09:31:41AM +0800, Hal Feng wrote:
> > This patch series adds temperature sensor support for StarFive JH7110 SoC.
> > The last two patches depend on series [1].
> > 
> > [1]: https://lore.kernel.org/all/20221220011247.35560-1-hal.feng@starfivetech.com/
> > 
> > Emil Renner Berthing (4):
> >   dt-bindings: hwmon: Add starfive,jh71x0-temp
> >   hwmon: (sfctemp) Add StarFive JH71x0 temperature sensor
> >   riscv: dts: starfive: jh7110: Add temperature sensor node
> >   riscv: dts: starfive: visionfive-2: Add thermal-zones
> > 
> 
> The hardware monitoring driver is obviously either the same
> or derived from the previous series at
> https://patchwork.kernel.org/project/linux-hwmon/list/?series=&submitter=&state=*&q=starfive
> 
> Why is this not submitted as v4 of the original series ?
> What has changed, and what is the rationale for (re-)submitting
> it as v1 ?

Sorry for the late reply. I feel sorry to say that I didn't know
the submitting history of this patch series and Emil forgot to
tell me maybe. After comparing with the previous series, I find
the changes between this one and the previous one can be concluded
as below.

Change log:
- Added support for StarFive JH7110 SoC besides JH7100.
- Added clock and reset support in dt-bindings and driver.
- Added two patches for adding nodes in JH7110 and VisionFive 2 dts
  which were being reviewed.

Thank you for your kindly reminding. I will resend this patch
series as version 4 and add the change log.

Best regards,
Hal

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

[Hal Feng]

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:
>> From: Emil Renner Berthing <kernel@esmil.dk>
>> 
>> Register definitions and conversion constants based on sfctemp driver by
>> Samin in the StarFive 5.10 kernel.
>> 
>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>> Co-developed-by: Samin Guo <samin.guo@starfivetech.com>
>> Signed-off-by: Samin Guo <samin.guo@starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> 
> This is obviously version 4 of the previous patch series,
> with various enhancements. Please do not "sell" that as
> v1 of a new patch series. Submit it as next version,
> and provide a change log.

Sorry to reply too late. I will resend this series as v4 tomorrow.

> 
>> ---
>>  Documentation/hwmon/index.rst   |   1 +
>>  Documentation/hwmon/sfctemp.rst |  33 +++
>>  MAINTAINERS                     |   8 +
>>  drivers/hwmon/Kconfig           |  10 +
>>  drivers/hwmon/Makefile          |   1 +
>>  drivers/hwmon/sfctemp.c         | 350 ++++++++++++++++++++++++++++++++
>>  6 files changed, 403 insertions(+)
>>  create mode 100644 Documentation/hwmon/sfctemp.rst
>>  create mode 100644 drivers/hwmon/sfctemp.c
>> 
>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>> index c1d11cf13eef..f7ede608b6e3 100644
>> --- a/Documentation/hwmon/index.rst
>> +++ b/Documentation/hwmon/index.rst
>> @@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers
>>     sch5627
>>     sch5636
>>     scpi-hwmon
>> +   sfctemp
>>     sht15
>>     sht21
>>     sht3x
>> diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst
>> new file mode 100644
>> index 000000000000..9fbd5bb1f356
>> --- /dev/null
>> +++ b/Documentation/hwmon/sfctemp.rst
>> @@ -0,0 +1,33 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +Kernel driver sfctemp
>> +=====================
>> +
>> +Supported chips:
>> + - StarFive JH7100
>> + - StarFive JH7110
>> +
>> +Authors:
>> + - Emil Renner Berthing <kernel@esmil.dk>
>> +
>> +Description
>> +-----------
>> +
>> +This driver adds support for reading the built-in temperature sensor on the
>> +JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd.
>> +
>> +``sysfs`` interface
>> +-------------------
>> +
>> +The temperature sensor can be enabled, disabled and queried via the standard
>> +hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of
>> +``X``:
>> +
>> +================ ==== =============================================
>> +Name             Perm Description
>> +================ ==== =============================================
>> +temp1_enable     RW   Enable or disable temperature sensor.
>> +                      Automatically enabled by the driver,
>> +                      but may be disabled to save power.
>> +temp1_input      RO   Temperature reading in milli-degrees Celsius.
>> +================ ==== =============================================
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 85e8f83161d7..ab3cd5827b26 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18661,6 +18661,14 @@ L:	netdev@vger.kernel.org
>>  S:	Supported
>>  F:	drivers/net/ethernet/sfc/
>>  
>> +SFCTEMP HWMON DRIVER
>> +M:	Emil Renner Berthing <kernel@esmil.dk>
>> +L:	linux-hwmon@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
>> +F:	Documentation/hwmon/sfctemp.rst
>> +F:	drivers/hwmon/sfctemp.c
>> +
>>  SFF/SFP/SFP+ MODULE SUPPORT
>>  M:	Russell King <linux@armlinux.org.uk>
>>  L:	netdev@vger.kernel.org
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 7ac3daaf59ce..c6bbfcca3a14 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1910,6 +1910,16 @@ config SENSORS_STTS751
>>  	  This driver can also be built as a module. If so, the module
>>  	  will be called stts751.
>>  
>> +config SENSORS_SFCTEMP
>> +	tristate "Starfive JH71x0 temperature sensor"
>> +	depends on SOC_STARFIVE || COMPILE_TEST
>> +	help
>> +	  If you say yes here you get support for temperature sensor
>> +	  on the Starfive JH71x0 SoCs.
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called sfctemp.
>> +
>>  config SENSORS_SMM665
>>  	tristate "Summit Microelectronics SMM665"
>>  	depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 11d076cad8a2..5a4a02c5535c 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI)	+= sbrmi.o
>>  obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>>  obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>>  obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
>> +obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
>>  obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>>  obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
>> 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.

Thank you for your feedback.

Best regards,
Hal

Re: [PATCH v1 1/4] dt-bindings: hwmon: Add starfive,jh71x0-temp

[Hal Feng]

On Tue, 3 Jan 2023 14:14:05 -0800, Guenter Roeck wrote:
> On Tue, Jan 03, 2023 at 09:31:42AM +0800, Hal Feng wrote:
>> From: Emil Renner Berthing <kernel@esmil.dk>
>> 
>> Add bindings for the temperature sensor on the StarFive JH7100 and
>> JH7110 SoCs.
>> 
>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> 
> The previous version of this bindings document (v3, 
> https://patchwork.kernel.org/project/linux-hwmon/patch/20210726171802.1052716-2-kernel@esmil.dk/
> had been reviewed by Rob Herring. Even though this version is named differently
> (starfive,jh71x0-temp.yaml instead of starfive,jh7100-temp.yaml), the old version
> should be referenced, and there should be a change log.

Will add the Reviewed-by tag of Rob and add a change log. Thank you again.

Best regards,
Hal

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

[Guenter Roeck]

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:
>>> From: Emil Renner Berthing <kernel@esmil.dk>
>>>
>>> Register definitions and conversion constants based on sfctemp driver by
>>> Samin in the StarFive 5.10 kernel.
>>>
>>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>>> Co-developed-by: Samin Guo <samin.guo@starfivetech.com>
>>> Signed-off-by: Samin Guo <samin.guo@starfivetech.com>
>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>
>> This is obviously version 4 of the previous patch series,
>> with various enhancements. Please do not "sell" that as
>> v1 of a new patch series. Submit it as next version,
>> and provide a change log.
> 
> Sorry to reply too late. I will resend this series as v4 tomorrow.
> 
>>
>>> ---
>>>   Documentation/hwmon/index.rst   |   1 +
>>>   Documentation/hwmon/sfctemp.rst |  33 +++
>>>   MAINTAINERS                     |   8 +
>>>   drivers/hwmon/Kconfig           |  10 +
>>>   drivers/hwmon/Makefile          |   1 +
>>>   drivers/hwmon/sfctemp.c         | 350 ++++++++++++++++++++++++++++++++
>>>   6 files changed, 403 insertions(+)
>>>   create mode 100644 Documentation/hwmon/sfctemp.rst
>>>   create mode 100644 drivers/hwmon/sfctemp.c
>>>
>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>>> index c1d11cf13eef..f7ede608b6e3 100644
>>> --- a/Documentation/hwmon/index.rst
>>> +++ b/Documentation/hwmon/index.rst
>>> @@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers
>>>      sch5627
>>>      sch5636
>>>      scpi-hwmon
>>> +   sfctemp
>>>      sht15
>>>      sht21
>>>      sht3x
>>> diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst
>>> new file mode 100644
>>> index 000000000000..9fbd5bb1f356
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/sfctemp.rst
>>> @@ -0,0 +1,33 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +Kernel driver sfctemp
>>> +=====================
>>> +
>>> +Supported chips:
>>> + - StarFive JH7100
>>> + - StarFive JH7110
>>> +
>>> +Authors:
>>> + - Emil Renner Berthing <kernel@esmil.dk>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver adds support for reading the built-in temperature sensor on the
>>> +JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd.
>>> +
>>> +``sysfs`` interface
>>> +-------------------
>>> +
>>> +The temperature sensor can be enabled, disabled and queried via the standard
>>> +hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of
>>> +``X``:
>>> +
>>> +================ ==== =============================================
>>> +Name             Perm Description
>>> +================ ==== =============================================
>>> +temp1_enable     RW   Enable or disable temperature sensor.
>>> +                      Automatically enabled by the driver,
>>> +                      but may be disabled to save power.
>>> +temp1_input      RO   Temperature reading in milli-degrees Celsius.
>>> +================ ==== =============================================
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 85e8f83161d7..ab3cd5827b26 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -18661,6 +18661,14 @@ L:	netdev@vger.kernel.org
>>>   S:	Supported
>>>   F:	drivers/net/ethernet/sfc/
>>>   
>>> +SFCTEMP HWMON DRIVER
>>> +M:	Emil Renner Berthing <kernel@esmil.dk>
>>> +L:	linux-hwmon@vger.kernel.org
>>> +S:	Maintained
>>> +F:	Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml
>>> +F:	Documentation/hwmon/sfctemp.rst
>>> +F:	drivers/hwmon/sfctemp.c
>>> +
>>>   SFF/SFP/SFP+ MODULE SUPPORT
>>>   M:	Russell King <linux@armlinux.org.uk>
>>>   L:	netdev@vger.kernel.org
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 7ac3daaf59ce..c6bbfcca3a14 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1910,6 +1910,16 @@ config SENSORS_STTS751
>>>   	  This driver can also be built as a module. If so, the module
>>>   	  will be called stts751.
>>>   
>>> +config SENSORS_SFCTEMP
>>> +	tristate "Starfive JH71x0 temperature sensor"
>>> +	depends on SOC_STARFIVE || COMPILE_TEST
>>> +	help
>>> +	  If you say yes here you get support for temperature sensor
>>> +	  on the Starfive JH71x0 SoCs.
>>> +
>>> +	  This driver can also be built as a module.  If so, the module
>>> +	  will be called sfctemp.
>>> +
>>>   config SENSORS_SMM665
>>>   	tristate "Summit Microelectronics SMM665"
>>>   	depends on I2C
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 11d076cad8a2..5a4a02c5535c 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI)	+= sbrmi.o
>>>   obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>>>   obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>>>   obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
>>> +obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
>>>   obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>>>   obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>>>   obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
>>> 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.

Guenter

> Thank you for your feedback.
> 
> Best regards,
> Hal

Re: [PATCH v1 0/4] Temperature sensor support for StarFive JH7110 RISC-V SoC

[Hal Feng]

On Fri, 6 Jan 2023 09:11:53 +0800, Hal Feng wrote:
> On Mon, 2 Jan 2023 17:56:01 -0800, Guenter Roeck wrote:
> > On Tue, Jan 03, 2023 at 09:31:41AM +0800, Hal Feng wrote:
> > > This patch series adds temperature sensor support for StarFive JH7110 SoC.
> > > The last two patches depend on series [1].
> > > 
> > > [1]: https://lore.kernel.org/all/20221220011247.35560-1-hal.feng@starfivetech.com/
> > > 
> > > Emil Renner Berthing (4):
> > >   dt-bindings: hwmon: Add starfive,jh71x0-temp
> > >   hwmon: (sfctemp) Add StarFive JH71x0 temperature sensor
> > >   riscv: dts: starfive: jh7110: Add temperature sensor node
> > >   riscv: dts: starfive: visionfive-2: Add thermal-zones
> > > 
> > 
> > The hardware monitoring driver is obviously either the same
> > or derived from the previous series at
> > https://patchwork.kernel.org/project/linux-hwmon/list/?series=&submitter=&state=*&q=starfive
> > 
> > Why is this not submitted as v4 of the original series ?
> > What has changed, and what is the rationale for (re-)submitting
> > it as v1 ?
> 
> Sorry for the late reply. I feel sorry to say that I didn't know
> the submitting history of this patch series and Emil forgot to
> tell me maybe. After comparing with the previous series, I find
> the changes between this one and the previous one can be concluded
> as below.
> 
> Change log:
> - Added support for StarFive JH7110 SoC besides JH7100.
> - Added clock and reset support in dt-bindings and driver.
> - Added two patches for adding nodes in JH7110 and VisionFive 2 dts
>   which were being reviewed.
> 
> Thank you for your kindly reminding. I will resend this patch
> series as version 4 and add the change log.

I have resent this series as v4 [1]. Even though there are some issues
need to be discussed with Guenter, I wanna resend this series in advance
so that the v4 can be mostly synchronized with the latest patches [2] [3]
from Emil. The new change will be added in the v5.

[1] https://lore.kernel.org/all/20230207072314.62040-1-hal.feng@starfivetech.com/
[2] https://github.com/esmil/linux/commit/8c7f2f00105384390ee02d745b6ccb6637e28d25
[3] https://github.com/esmil/linux/commit/d04731cf0dc0462bc76afbadf95366ff0edbe642

Best regards,
Hal

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

[Hal Feng]

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?
Without the interrupt support, the temperature we read may be the value
generated in the last cycle.

I think the temperature has its value only when we read it, so we start
conversion only when we read the temperature. Further more, it will
consume more power if we enable continuous mode.

Best regards,
Hal

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

[Guenter Roeck]

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

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

[Hal Feng]

On Wed, 8 Feb 2023 07:16:52 -0800, Guenter Roeck wrote:
> 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.

OK, I see. Thanks.

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

Because without the interrupt, there is no flag pointing out when the
conversion is finished, and the temperature we read is the data generated
before. With further consideration, the error is no more than one sample
time (8.192 ms) in continuous mode while the error is no less than one sample
time in single mode. In this respect, continuous mode is better than single
mode.

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

Sorry for my inexact statement. I means the temperature data is useful to us
only when we read it. If we don't read it, the temperature data will just be
discarded every sample time.

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

After some test, it's found that there is almost no difference between these
two modes in power consumption.

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

It's OK to run the continuous mode instead and remove the "- interrupts" entry
in "required:" of DT bindings. I will modify in the next version. Thank you
for your suggestions.

Best regards,
Hal