* [PATCH 1/5] units: add a macro for MILLIVOLT_PER_VOLT
@ 2023-04-10 15:33 dinh.nguyen
2023-04-10 15:33 ` [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA dinh.nguyen
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: dinh.nguyen @ 2023-04-10 15:33 UTC (permalink / raw)
To: linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, linux, Dinh Nguyen, Andy Shevchenko
From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
Add a define for MILLIVOLT_PER_VOLT.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
---
include/linux/units.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/units.h b/include/linux/units.h
index 2793a41e73a2..5b797535e1b9 100644
--- a/include/linux/units.h
+++ b/include/linux/units.h
@@ -31,6 +31,8 @@
#define MICROWATT_PER_MILLIWATT 1000UL
#define MICROWATT_PER_WATT 1000000UL
+#define MILLIVOLT_PER_VOLT 1000UL
+
#define ABSOLUTE_ZERO_MILLICELSIUS -273150
static inline long milli_kelvin_to_millicelsius(long t)
--
2.40.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA
2023-04-10 15:33 [PATCH 1/5] units: add a macro for MILLIVOLT_PER_VOLT dinh.nguyen
@ 2023-04-10 15:33 ` dinh.nguyen
2023-04-10 23:48 ` Rob Herring
2023-04-11 6:53 ` Krzysztof Kozlowski
2023-04-10 15:33 ` [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms dinh.nguyen
` (2 subsequent siblings)
3 siblings, 2 replies; 14+ messages in thread
From: dinh.nguyen @ 2023-04-10 15:33 UTC (permalink / raw)
To: linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, linux, Dinh Nguyen
From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
Document the hardware monitoring bindings for SoCFPGA 64-bit platforms.
Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
---
.../bindings/hwmon/intel,socfpga-hwmon.yaml | 241 ++++++++++++++++++
1 file changed, 241 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
new file mode 100644
index 000000000000..ec9d9eabdc37
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
@@ -0,0 +1,241 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/intel,socfpga-hwmon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel SoCFPGA Hardware monitor
+
+maintainers:
+ - Dinh Nguyen <dinh.nguyen@linux.intel.com>
+
+description: |
+ The Intel SoCFPGA hardware monitor unit provides on-chip voltage and
+ temperature sensors. You can use these sensors to monitor external
+ voltages and on-chip operating conditions such as internal power rails
+ and on-chip junction temperatures.
+
+ The specific sensor configurations vary for each device family and
+ each device within a family does not offer all potential sensor
+ options. The information below attempts to illustrate the super set of
+ possible sensor options that are potentially available within each
+ device family, but the user should check the documentation for the
+ specific device they are using to verify which sensor options it
+ actually provides.
+
+ Stratix 10 Device Family
+
+ Stratix 10 Voltage Sensors
+
+ page 0, channel 2 = 0.8V VCC
+ page 0, channel 3 = 1.0V VCCIO
+ page 0, channel 6 = 0.9V VCCERAM
+
+ Stratix 10 Temperature Sensors
+
+ page 0, channel 0 = main die
+ page 0, channel 1 = tile bottom left
+ page 0, channel 2 = tile middle left
+ page 0, channel 3 = tile top left
+ page 0, channel 4 = tile bottom right
+ page 0, channel 5 = tile middle right
+ page 0, channel 6 = tile top right
+ page 0, channel 7 = hbm2 bottom
+ page 0, channel 8 = hbm2 top
+
+ Agilex Device Family
+
+ Agilex Voltage Sensors
+
+ page 0, channel 2 = 0.8V VCC
+ page 0, channel 3 = 1.8V VCCIO_SDM
+ page 0, channel 4 = 1.8V VCCPT
+ page 0, channel 5 = 1.2V VCCRCORE
+ page 0, channel 6 = 0.9V VCCH
+ page 0, channel 7 = 0.8V VCCL
+
+ Agilex Temperature Sensors
+
+ page 0, channel 0 = main die sdm max
+ page 0, channel 1 = main die sdm 1
+
+ page 1, channel 0 = main die corner bottom left max
+ page 1, channel 1 = main die corner bottom left 1
+ page 1, channel 2 = main die corner bottom left 2
+
+ page 2, channel 0 = main die corner top left max
+ page 2, channel 1 = main die corner top left 1
+ page 2, channel 2 = main die corner top left 2
+
+ page 3, channel 0 = main die corner bottom right max
+ page 3, channel 1 = main die corner bottom right 1
+ page 3, channel 2 = main die corner bottom right 2
+
+ page 4, channel 0 = main die corner top right max
+ page 4, channel 1 = main die corner top right 1
+ page 4, channel 2 = main die corner top right 2
+
+ page 5, channel 0 = tile die bottom left max
+ page 5, channel 1 = tile die bottom left 1
+ page 5, channel 6..2 = tile die bottom left 6..2 R-tile only
+ page 5, channel 5..2 = tile die bottom left 5..2 F-tile only
+ page 5, channel 4..2 = tile die bottom left 4..2 E-tile only
+
+ page 7, channel 0 = tile die top left max
+ page 7, channel 1 = tile die top left 1
+ page 7, channel 6..2 = tile die top left 6..2 R-tile only
+ page 7, channel 5..2 = tile die top left 5..2 F-tile only
+ page 7, channel 4..2 = tile die top left 4..2 E-tile only
+
+ page 8, channel 0 = tile die bottom right max
+ page 8, channel 1 = tile die bottom right 1
+ page 8, channel 6..2 = tile die bottom right 6..2 R-tile only
+ page 8, channel 5..2 = tile die bottom right 5..2 F-tile only
+ page 8, channel 4..2 = tile die bottom right 4..2 E-tile only
+
+ page 10, channel 0 = tile die top right max
+ page 10, channel 1 = tile die top right 1
+ page 10, channel 6..2 = tile die top right 6..2 R-tile only
+ page 10, channel 5..2 = tile die top right 5..2 F-tile only
+ page 10, channel 4..2 = tile die top right 4..2 E-tile only
+
+ N5X Device Family
+
+ N5X Voltage Sensors
+
+ page 0, channel 2 = 0.8V VDD
+ page 0, channel 3 = 0.8V VDD_SDM
+ page 0, channel 4 = 1.8V VCCADC
+ page 0, channel 5 = 1.8V VCCPD
+ page 0, channel 6 = 1.8V VCCIO_SDM
+ page 0, channel 7 = 0.8V VDD_HPS
+
+ N5X Temperature Sensors
+
+ page 0, channel 0 = main die
+
+properties:
+ compatible:
+ enum:
+ - intel,socfpga-hwmon
+
+ reg:
+ maxItems: 1
+ description:
+ The sensor mapping address is denoted by the lower 16-bits being
+ the channel mask location that defines the channel number.
+ The upper 16-bits denotes the page number.
+ The bit mask of 0x1 represents channel 1. The supported
+ page and channel is dependent on the SoCFPGA variant.
+ Page number greater than 0 is only supported on the
+ temperature sensors.
+
+ temperature:
+ description:
+ Specifies the possible mappings of temperature sensors
+ diodes on the SoCFPGA main die and tile die.
+
+ voltage:
+ description:
+ Specifies the possible mappings of the voltage sensors on the
+ SoCFPGA analog to digital converter of the Secure Device Manager
+ (SDM).
+
+ input:
+ description:
+ Specifies each sensor.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ temp_volt {
+ compatible = "intel,socfpga-hwmon";
+ voltage {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ input@2 {
+ label = "0.8V VCC";
+ reg = <2>;
+ };
+
+ input@3 {
+ label = "1.8V VCCIO_SDM";
+ reg = <3>;
+ };
+
+ input@4 {
+ label = "1.8V VCCPT";
+ reg = <4>;
+ };
+
+ input@5 {
+ label = "1.2V VCCCRCORE";
+ reg = <5>;
+ };
+
+ input@6 {
+ label = "0.9V VCCH";
+ reg = <6>;
+ };
+
+ input@7 {
+ label = "0.8V VCCL";
+ reg = <7>;
+ };
+ };
+
+ temperature {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ input@0 {
+ label = "Main Die SDM";
+ reg = <0x0>;
+ };
+
+ input@10001 {
+ label = "Main Die corner bottom left 1";
+ reg = <0x10001>;
+ };
+
+ input@10002 {
+ label = "Main Die corner bottom left 2";
+ reg = <0x10002>;
+ };
+
+ input@20001 {
+ label = "Main Die corner top left 1";
+ reg = <0x20001>;
+ };
+
+ input@20002 {
+ label = "Main Die corner top left 2";
+ reg = <0x20002>;
+ };
+
+ input@30001 {
+ label = "Main Die corner bottom right 1";
+ reg = <0x30001>;
+ };
+
+ input@30002 {
+ label = "Main Die corner bottom right 2";
+ reg = <0x30002>;
+ };
+
+ input@40001 {
+ label = "Main Die corner top right 1 HPS";
+ reg = <0x40001>;
+ };
+
+ input@40002 {
+ label = "Main Die corner top right 2";
+ reg = <0x40002>;
+ };
+ };
+ };
--
2.40.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-10 15:33 [PATCH 1/5] units: add a macro for MILLIVOLT_PER_VOLT dinh.nguyen
2023-04-10 15:33 ` [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA dinh.nguyen
@ 2023-04-10 15:33 ` dinh.nguyen
2023-04-11 2:44 ` Guenter Roeck
2023-04-10 15:33 ` [PATCH 4/5] MAINTAINERS: add Dinh Nguyen for socfpga-hwmon driver dinh.nguyen
2023-04-10 15:33 ` [PATCH 5/5] arm64: dts: socfpga: add hwmon properties dinh.nguyen
3 siblings, 1 reply; 14+ messages in thread
From: dinh.nguyen @ 2023-04-10 15:33 UTC (permalink / raw)
To: linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, linux, Dinh Nguyen, Andy Shevchenko
From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
The driver supports 64-bit SoCFPGA platforms for temperature and voltage
reading using the platform's SDM(Secure Device Manager). The driver
also uses the Stratix10 Service layer driver.
This driver only supports OF SoCFPGA 64-bit platforms.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/socfpga-hwmon.rst | 30 ++
drivers/firmware/stratix10-svc.c | 18 +-
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/socfpga-hwmon.c | 406 ++++++++++++++++++
include/linux/firmware/intel/stratix10-smc.h | 34 ++
.../firmware/intel/stratix10-svc-client.h | 6 +
8 files changed, 506 insertions(+), 1 deletion(-)
create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
create mode 100644 drivers/hwmon/socfpga-hwmon.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index f1fe75f596a5..9db4e1537481 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -196,6 +196,7 @@ Hardware Monitoring Kernel Drivers
smsc47b397
smsc47m192
smsc47m1
+ socfpga-hwmon
sparx5-temp
stpddc60
sy7636a-hwmon
diff --git a/Documentation/hwmon/socfpga-hwmon.rst b/Documentation/hwmon/socfpga-hwmon.rst
new file mode 100644
index 000000000000..f6565c83cf40
--- /dev/null
+++ b/Documentation/hwmon/socfpga-hwmon.rst
@@ -0,0 +1,30 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Kernel driver socfpga-hwmon
+===========================
+
+Supported chips:
+
+ * Intel Stratix10
+ * Intel Agilex
+ * Intel N5X
+
+Author: Dinh Nguyen <dinh.nguyen@linux.intel.com>
+
+Description
+-----------
+
+This driver supports hardware monitoring for 64-Bit SoCFPGA and eASIC devices
+based around the Secure Device Manager and Stratix 10 Service layer.
+
+The following sensor types are supported:
+
+ * temperature
+ * voltage
+
+Usage Notes
+-----------
+
+The driver relies on a device tree node to enumerate support present on the
+specific device. See Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
+for details of the device-tree node.
diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index bde1f543f529..cc1b8b441c37 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -340,6 +340,8 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
case COMMAND_RSU_MAX_RETRY:
case COMMAND_RSU_DCMF_STATUS:
case COMMAND_FIRMWARE_VERSION:
+ case COMMAND_HWMON_READTEMP:
+ case COMMAND_HWMON_READVOLT:
cb_data->status = BIT(SVC_STATUS_OK);
cb_data->kaddr1 = &res.a1;
break;
@@ -517,7 +519,17 @@ static int svc_normal_to_secure_thread(void *data)
a1 = (unsigned long)pdata->paddr;
a2 = 0;
break;
-
+ /* for HWMON */
+ case COMMAND_HWMON_READTEMP:
+ a0 = INTEL_SIP_SMC_HWMON_READTEMP;
+ a1 = pdata->arg[0];
+ a2 = 0;
+ break;
+ case COMMAND_HWMON_READVOLT:
+ a0 = INTEL_SIP_SMC_HWMON_READVOLT;
+ a1 = pdata->arg[0];
+ a2 = 0;
+ break;
/* for polling */
case COMMAND_POLL_SERVICE_STATUS:
a0 = INTEL_SIP_SMC_SERVICE_COMPLETED;
@@ -1182,6 +1194,10 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
chans[2].name = SVC_CLIENT_FCS;
spin_lock_init(&chans[2].lock);
+ chans[3].ctrl = controller;
+ chans[3].name = SVC_CLIENT_HWMON;
+ spin_lock_init(&chans[3].lock);
+
list_add_tail(&controller->node, &svc_ctrl);
platform_set_drvdata(pdev, controller);
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5b3b76477b0e..c7c978acfece 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1875,6 +1875,17 @@ config SENSORS_SMSC47M192
This driver can also be built as a module. If so, the module
will be called smsc47m192.
+config SENSORS_SOCFPGA
+ tristate "SoCFPGA Hardware monitoring features"
+ depends on INTEL_STRATIX10_SERVICE
+ depends on OF || COMPILE_TEST
+ help
+ If you say yes here you get support for the temperature and
+ voltage sensors of 64-bit SoCFPGA devices.
+
+ This driver can also be built as a module. If so, the module
+ will be called socfpga-hwmon.
+
config SENSORS_SMSC47B397
tristate "SMSC LPC47B397-NC"
depends on !PPC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 88712b5031c8..c04c0b2578a4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -193,6 +193,7 @@ obj-$(CONFIG_SENSORS_SMPRO) += smpro-hwmon.o
obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
+obj-$(CONFIG_SENSORS_SOCFPGA) += socfpga-hwmon.o
obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
obj-$(CONFIG_SENSORS_STTS751) += stts751.o
obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c
new file mode 100644
index 000000000000..636e6e269578
--- /dev/null
+++ b/drivers/hwmon/socfpga-hwmon.c
@@ -0,0 +1,406 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SoCFPGA hardware monitoring features
+ *
+ * Copyright (c) 2023 Intel Corporation. All rights reserved
+ */
+#include <linux/arm-smccc.h>
+#include <linux/hwmon.h>
+#include <linux/firmware/intel/stratix10-svc-client.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/units.h>
+
+#define HWMON_TIMEOUT msecs_to_jiffies(SVC_HWMON_REQUEST_TIMEOUT_MS)
+
+/*
+ * When bit 31 is set, an error condition has been detected in the
+ * temperature sensor.
+ */
+#define ETEMP_ERROR BIT(31)
+/*
+ * Selected temperature sensor channel is currently inactive.
+ * Ensure that the tile where the TSD is located is actively in use.
+ */
+#define ETEMP_INACTIVE 0x0
+/*
+ * Selected temperature sensor channel returned a value that is not the
+ * latest reading. Try retrieve the temperature reading again.
+ */
+#define ETEMP_TOO_OLD 0x1
+/*
+ * Selected temperature sensor channel is invalid for the device. Ignore
+ * the returned data because the temperature sensor channel location is
+ * invalid.
+ */
+#define ETEMP_NOT_PRESENT 0x2
+/*
+ * System is corrupted or failed to respond.
+ */
+#define ETEMP_TIMEOUT 0x3
+#define ETEMP_CORRUPT 0x4
+/*
+ * Communication mechanism is busy.
+ */
+#define ETEMP_BUSY 0x5
+/*
+ * System is corrupted or failed to respond.
+ */
+#define ETEMP_NOT_INITIALIZED 0xFF
+
+#define SOCFPGA_HWMON_MAXSENSORS 16
+
+/**
+ * struct socfpga_hwmon_chan - channel input parameters.
+ * @n : Number of channels.
+ * @value: value read from the chip.
+ * @names: names array from DTS labels.
+ * @chan: channel array.
+ *
+ * The structure represents either the voltage or temperature information
+ * for the hwmon channels.
+ */
+struct socfpga_hwmon_chan {
+ unsigned int n;
+ int value;
+ const char *names[SOCFPGA_HWMON_MAXSENSORS];
+ u32 chan[SOCFPGA_HWMON_MAXSENSORS];
+};
+
+struct socfpga_hwmon_priv {
+ struct stratix10_svc_client client;
+ struct stratix10_svc_client_msg msg;
+ struct stratix10_svc_chan *chan;
+ struct completion completion;
+ struct mutex lock;
+ struct socfpga_hwmon_chan temperature;
+ struct socfpga_hwmon_chan voltage;
+};
+
+enum hwmon_type_op {
+ SOCFPGA_HWMON_TYPE_TEMP,
+ SOCFPGA_HWMON_TYPE_VOLT,
+ SOCFPGA_HWMON_TYPE_MAX
+};
+
+static const char *const hwmon_types_str[] = { "temperature", "voltage" };
+
+static umode_t socfpga_is_visible(const void *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int chan)
+{
+ switch (type) {
+ case hwmon_temp:
+ case hwmon_in:
+ return 0444;
+ default:
+ return 0;
+ }
+}
+
+static void socfpga_smc_callback(struct stratix10_svc_client *client,
+ struct stratix10_svc_cb_data *data)
+{
+ struct socfpga_hwmon_priv *priv = client->priv;
+ struct arm_smccc_res *res = data->kaddr1;
+
+ if (data->status == BIT(SVC_STATUS_OK)) {
+ if (priv->msg.command == COMMAND_HWMON_READTEMP)
+ priv->temperature.value = res->a0;
+ else
+ priv->voltage.value = res->a0;
+ } else
+ dev_err(client->dev, "%s returned 0x%lX\n", __func__, res->a0);
+
+ complete(&priv->completion);
+}
+
+static int socfpga_hwmon_send(struct socfpga_hwmon_priv *priv)
+{
+ int ret;
+
+ priv->client.receive_cb = socfpga_smc_callback;
+
+ ret = stratix10_svc_send(priv->chan, &priv->msg);
+ if (ret < 0)
+ return ret;
+
+ if (!wait_for_completion_timeout(&priv->completion, HWMON_TIMEOUT)) {
+ dev_err(priv->client.dev, "SMC call timeout!\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
+{
+ int value = priv->temperature.value;
+
+ if (!(value & ETEMP_ERROR))
+ return 0;
+
+ dev_err(priv->client.dev, "temperature sensor code 0x%08x\n", value);
+
+ value &= ~ETEMP_ERROR;
+ switch (value) {
+ case ETEMP_NOT_PRESENT:
+ return -ENOENT;
+ case ETEMP_CORRUPT:
+ case ETEMP_NOT_INITIALIZED:
+ return -ENODATA;
+ case ETEMP_BUSY:
+ return -EBUSY;
+ case ETEMP_INACTIVE:
+ case ETEMP_TIMEOUT:
+ case ETEMP_TOO_OLD:
+ return -EAGAIN;
+ default:
+ /* Unknown error */
+ return -EINVAL;
+ }
+}
+
+static int socfpga_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int chan, long *val)
+{
+ struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(&priv->lock);
+ reinit_completion(&priv->completion);
+
+ switch (type) {
+ case hwmon_temp:
+ priv->msg.arg[0] = BIT_ULL(priv->temperature.chan[chan]);
+ priv->msg.command = COMMAND_HWMON_READTEMP;
+ if (socfpga_hwmon_send(priv))
+ goto status_done;
+
+ ret = socfpga_hwmon_err_to_errno(priv);
+ if (ret)
+ break;
+ /*
+ * The Temperature Sensor IP core returns the Celsius
+ * temperature value in signed 32-bit fixed point binary
+ * format, with eight bits below binary point.
+ */
+ *val = (priv->temperature.value * MILLIDEGREE_PER_DEGREE) / 256;
+ break;
+ case hwmon_in: /* Read voltage */
+ priv->msg.arg[0] = BIT_ULL(priv->voltage.chan[chan]);
+ priv->msg.command = COMMAND_HWMON_READVOLT;
+ if (socfpga_hwmon_send(priv))
+ goto status_done;
+
+ /*
+ * The Voltage Sensor IP core returns the sampled voltage
+ * in unsigned 32-bit fixed point binary format, with 16 bits
+ * below binary point.
+ */
+ *val = (priv->voltage.value * MILLIVOLT_PER_VOLT) / 65536;
+ break;
+ default:
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+status_done:
+ stratix10_svc_done(priv->chan);
+ mutex_unlock(&priv->lock);
+ return ret;
+}
+
+static int socfpga_read_string(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int chan, const char **str)
+{
+ struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_in:
+ *str = priv->voltage.names[chan];
+ return 0;
+ case hwmon_temp:
+ *str = priv->temperature.names[chan];
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static const struct hwmon_ops socfpga_ops = {
+ .is_visible = socfpga_is_visible,
+ .read = socfpga_read,
+ .read_string = socfpga_read_string,
+};
+
+static const struct hwmon_channel_info *socfpga_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL),
+ HWMON_CHANNEL_INFO(in,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL),
+ NULL
+};
+
+static const struct hwmon_chip_info socfpga_chip_info = {
+ .ops = &socfpga_ops,
+ .info = socfpga_info,
+};
+
+static int socfpga_add_channel(struct device *dev, const char *type,
+ u32 val, const char *label,
+ struct socfpga_hwmon_priv *priv)
+{
+ int type_index;
+ struct socfpga_hwmon_chan *p;
+
+ type_index = match_string(hwmon_types_str, ARRAY_SIZE(hwmon_types_str), type);
+ switch (type_index) {
+ case SOCFPGA_HWMON_TYPE_TEMP:
+ p = &priv->temperature;
+ break;
+ case SOCFPGA_HWMON_TYPE_VOLT:
+ p = &priv->voltage;
+ break;
+ default:
+ return -ENODATA;
+ }
+ if (p->n >= SOCFPGA_HWMON_MAXSENSORS)
+ return -ENOSPC;
+
+ p->names[p->n] = label;
+ p->chan[p->n] = val;
+ p->n++;
+
+ return 0;
+}
+
+static int socfpga_probe_child_from_dt(struct device *dev,
+ struct device_node *child,
+ struct socfpga_hwmon_priv *priv)
+{
+ struct device_node *grandchild;
+ const char *label;
+ const char *type;
+ u32 val;
+ int ret;
+
+ if (of_property_read_string(child, "name", &type))
+ return dev_err_probe(dev, -EINVAL, "No type for %pOF\n", child);
+
+ for_each_child_of_node(child, grandchild) {
+ ret = of_property_read_u32(grandchild, "reg", &val);
+ if (ret)
+ return dev_err_probe(dev, ret, "missing reg property of %pOF\n",
+ grandchild);
+
+ ret = of_property_read_string(grandchild, "label", &label);
+ if (ret)
+ return dev_err_probe(dev, ret, "missing label propoerty of %pOF\n",
+ grandchild);
+ ret = socfpga_add_channel(dev, type, val, label, priv);
+ if (ret == -ENOSPC)
+ return dev_err_probe(dev, ret, "too many channels, only %d supported\n",
+ SOCFPGA_HWMON_MAXSENSORS);
+ }
+ return 0;
+}
+
+static int socfpga_probe_from_dt(struct device *dev,
+ struct socfpga_hwmon_priv *priv)
+{
+ const struct device_node *np = dev->of_node;
+ struct device_node *child;
+ int ret = 0;
+
+ for_each_child_of_node(np, child) {
+ ret = socfpga_probe_child_from_dt(dev, child, priv);
+ if (ret)
+ break;
+ }
+ of_node_put(child);
+
+ return ret;
+}
+
+static int socfpga_hwmon_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device *hwmon_dev;
+ struct socfpga_hwmon_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->client.dev = dev;
+ priv->client.priv = priv;
+
+ ret = socfpga_probe_from_dt(dev, priv);
+ if (ret)
+ return dev_err_probe(dev, ret, "Unable to probe from device tree\n");
+
+ mutex_init(&priv->lock);
+ init_completion(&priv->completion);
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpgahwmon",
+ priv,
+ &socfpga_chip_info,
+ NULL);
+ if (IS_ERR(hwmon_dev))
+ return PTR_ERR(hwmon_dev);
+
+ priv->chan = stratix10_svc_request_channel_byname(&priv->client,
+ SVC_CLIENT_HWMON);
+ if (IS_ERR(priv->chan))
+ return dev_err_probe(dev, PTR_ERR(priv->chan),
+ "couldn't get service channel %s\n",
+ SVC_CLIENT_RSU);
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int socfpga_hwmon_remove(struct platform_device *pdev)
+{
+ struct socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
+
+ stratix10_svc_free_channel(priv->chan);
+ return 0;
+}
+
+static const struct of_device_id socfpga_of_match[] = {
+ { .compatible = "intel,socfpga-hwmon" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, socfpga_of_match);
+
+static struct platform_driver socfpga_hwmon_driver = {
+ .driver = {
+ .name = "socfpga-hwmon",
+ .of_match_table = socfpga_of_match,
+ },
+ .probe = socfpga_hwmon_probe,
+ .remove = socfpga_hwmon_remove,
+};
+module_platform_driver(socfpga_hwmon_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("SoCFPGA hardware monitoring features");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/linux/firmware/intel/stratix10-smc.h
index a718f853d457..b944ec4b2b2f 100644
--- a/include/linux/firmware/intel/stratix10-smc.h
+++ b/include/linux/firmware/intel/stratix10-smc.h
@@ -595,4 +595,38 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
#define INTEL_SIP_SMC_FCS_GET_PROVISION_DATA \
INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FCS_GET_PROVISION_DATA)
+/**
+ * Request INTEL_SIP_SMC_HWMON_READTEMP
+ * Sync call to request temperature
+ *
+ * Call register usage:
+ * a0 Temperature Channel
+ * a1-a7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK
+ * a1 Temperature Value
+ * a2-a3 not used
+ */
+#define INTEL_SIP_SMC_FUNCID_HWMON_READTEMP 32
+#define INTEL_SIP_SMC_HWMON_READTEMP \
+ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_HWMON_READTEMP)
+
+/**
+ * Request INTEL_SIP_SMC_HWMON_READVOLT
+ * Sync call to request voltage
+ *
+ * Call register usage:
+ * a0 Voltage Channel
+ * a1-a7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK
+ * a1 Voltage Value
+ * a2-a3 not used
+ */
+#define INTEL_SIP_SMC_FUNCID_HWMON_READVOLT 33
+#define INTEL_SIP_SMC_HWMON_READVOLT \
+ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_HWMON_READVOLT)
+
#endif
diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index 0c16037fd08d..343970dcc2d2 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -11,10 +11,12 @@
*
* fpga: for FPGA configuration
* rsu: for remote status update
+ * hwmon: for hardware monitoring (voltage and temperature)
*/
#define SVC_CLIENT_FPGA "fpga"
#define SVC_CLIENT_RSU "rsu"
#define SVC_CLIENT_FCS "fcs"
+#define SVC_CLIENT_HWMON "hwmon"
/*
* Status of the sent command, in bit number
@@ -70,6 +72,7 @@
#define SVC_RSU_REQUEST_TIMEOUT_MS 300
#define SVC_FCS_REQUEST_TIMEOUT_MS 2000
#define SVC_COMPLETED_TIMEOUT_MS 30000
+#define SVC_HWMON_REQUEST_TIMEOUT_MS 300
struct stratix10_svc_chan;
@@ -164,6 +167,9 @@ enum stratix10_svc_command_code {
COMMAND_FCS_RANDOM_NUMBER_GEN,
/* for general status poll */
COMMAND_POLL_SERVICE_STATUS = 40,
+ /* for HWMON */
+ COMMAND_HWMON_READTEMP,
+ COMMAND_HWMON_READVOLT,
/* Non-mailbox SMC Call */
COMMAND_SMC_SVC_VERSION = 200,
};
--
2.40.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/5] MAINTAINERS: add Dinh Nguyen for socfpga-hwmon driver
2023-04-10 15:33 [PATCH 1/5] units: add a macro for MILLIVOLT_PER_VOLT dinh.nguyen
2023-04-10 15:33 ` [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA dinh.nguyen
2023-04-10 15:33 ` [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms dinh.nguyen
@ 2023-04-10 15:33 ` dinh.nguyen
2023-04-10 15:33 ` [PATCH 5/5] arm64: dts: socfpga: add hwmon properties dinh.nguyen
3 siblings, 0 replies; 14+ messages in thread
From: dinh.nguyen @ 2023-04-10 15:33 UTC (permalink / raw)
To: linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, linux, Dinh Nguyen
From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
Add Dinh Nguyen as the maintainer for the SoCFPGA hardware monitor
driver.
Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d5bc223f305..064f9db7ffe1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2781,6 +2781,12 @@ M: Dinh Nguyen <dinguyen@kernel.org>
S: Maintained
F: drivers/edac/altera_edac.[ch]
+ARM/SOCFPGA HARDWARE MONITOR
+M: Dinh Nguyen <dinh.nguyen@linux.intel.com>
+S: Maintained
+F: drivers/hwmon/socfpga-hwmon.c
+F: Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
+
ARM/SPREADTRUM SoC SUPPORT
M: Orson Zhai <orsonzhai@gmail.com>
M: Baolin Wang <baolin.wang7@gmail.com>
--
2.40.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/5] arm64: dts: socfpga: add hwmon properties
2023-04-10 15:33 [PATCH 1/5] units: add a macro for MILLIVOLT_PER_VOLT dinh.nguyen
` (2 preceding siblings ...)
2023-04-10 15:33 ` [PATCH 4/5] MAINTAINERS: add Dinh Nguyen for socfpga-hwmon driver dinh.nguyen
@ 2023-04-10 15:33 ` dinh.nguyen
3 siblings, 0 replies; 14+ messages in thread
From: dinh.nguyen @ 2023-04-10 15:33 UTC (permalink / raw)
To: linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, linux, Dinh Nguyen
From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
Add the hardware monitoring properties for Stratix10 and Agilex.
Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
---
.../boot/dts/altera/socfpga_stratix10.dtsi | 4 ++
.../dts/altera/socfpga_stratix10_socdk.dts | 31 +++++++++
arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 4 ++
.../boot/dts/intel/socfpga_agilex_n6000.dts | 66 +++++++++++++++++++
.../boot/dts/intel/socfpga_agilex_socdk.dts | 66 +++++++++++++++++++
.../dts/intel/socfpga_agilex_socdk_nand.dts | 66 +++++++++++++++++++
.../boot/dts/intel/socfpga_n5x_socdk.dts | 46 +++++++++++++
7 files changed, 283 insertions(+)
diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index 41c9eb51d0ee..0efb570d27e5 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -633,6 +633,10 @@ svc {
fpga_mgr: fpga-mgr {
compatible = "intel,stratix10-soc-fpga-mgr";
};
+
+ temp_volt: hwmon {
+ compatible = "intel,socfpga-hwmon";
+ };
};
};
};
diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
index 38ae674f2f02..eb0880a49f77 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
@@ -212,3 +212,34 @@ qspi_rootfs: partition@3FE0000 {
};
};
};
+
+&temp_volt {
+ voltage {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ input@2 {
+ label = "0.8V VCC";
+ reg = <2>;
+ };
+
+ input@3 {
+ label = "1.0V VCCIO";
+ reg = <3>;
+ };
+
+ input@6 {
+ label = "0.9V VCCERAM";
+ reg = <6>;
+ };
+ };
+
+ temperature {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ input@0 {
+ label = "Main Die SDM";
+ reg = <0x0>;
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
index f9674cc46764..d6cc52a48599 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
@@ -666,6 +666,10 @@ svc {
fpga_mgr: fpga-mgr {
compatible = "intel,agilex-soc-fpga-mgr";
};
+
+ temp_volt: hwmon {
+ compatible = "intel,socfpga-hwmon";
+ };
};
};
};
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts b/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
index 6231a69204b1..09ce00fe42d1 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
@@ -64,3 +64,69 @@ &watchdog0 {
&fpga_mgr {
status = "disabled";
};
+
+&temp_volt {
+ voltage {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ input@2 {
+ label = "0.8V VCC";
+ reg = <2>;
+ };
+
+ input@3 {
+ label = "1.8V VCCIO_SDM";
+ reg = <3>;
+ };
+
+ input@4 {
+ label = "1.8V VCCPT";
+ reg = <4>;
+ };
+
+ input@5 {
+ label = "1.2V VCCCRCORE";
+ reg = <5>;
+ };
+
+ input@6 {
+ label = "0.9V VCCH";
+ reg = <6>;
+ };
+
+ input@7 {
+ label = "0.8V VCCL";
+ reg = <7>;
+ };
+ };
+
+ temperature {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ input@0 {
+ label = "Main Die SDM";
+ reg = <0x0>;
+ };
+
+ input@10000 {
+ label = "Main Die corner bottom left max";
+ reg = <0x10000>;
+ };
+
+ input@20000 {
+ label = "Main Die corner top left max";
+ reg = <0x20000>;
+ };
+
+ input@30000 {
+ label = "Main Die corner bottom right max";
+ reg = <0x30000>;
+ };
+
+ input@40000 {
+ label = "Main Die corner top right max";
+ reg = <0x40000>;
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts b/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts
index 07c3f8876613..9af029e5633e 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts
@@ -138,3 +138,69 @@ qspi_rootfs: partition@3FE0000 {
};
};
};
+
+&temp_volt {
+ voltage {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ input@2 {
+ label = "0.8V VCC";
+ reg = <2>;
+ };
+
+ input@3 {
+ label = "1.8V VCCIO_SDM";
+ reg = <3>;
+ };
+
+ input@4 {
+ label = "1.8V VCCPT";
+ reg = <4>;
+ };
+
+ input@5 {
+ label = "1.2V VCCCRCORE";
+ reg = <5>;
+ };
+
+ input@6 {
+ label = "0.9V VCCH";
+ reg = <6>;
+ };
+
+ input@7 {
+ label = "0.8V VCCL";
+ reg = <7>;
+ };
+ };
+
+ temperature {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ input@0 {
+ label = "Main Die SDM";
+ reg = <0x0>;
+ };
+
+ input@10000 {
+ label = "Main Die corner bottom left max";
+ reg = <0x10000>;
+ };
+
+ input@20000 {
+ label = "Main Die corner top left max";
+ reg = <0x20000>;
+ };
+
+ input@30000 {
+ label = "Main Die corner bottom right max";
+ reg = <0x30000>;
+ };
+
+ input@40000 {
+ label = "Main Die corner top right max";
+ reg = <0x40000>;
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_socdk_nand.dts b/arch/arm64/boot/dts/intel/socfpga_agilex_socdk_nand.dts
index 51f83f96ec65..d3576bb8b04d 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex_socdk_nand.dts
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex_socdk_nand.dts
@@ -114,3 +114,69 @@ &usb0 {
&watchdog0 {
status = "okay";
};
+
+&temp_volt {
+ voltage {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ input@2 {
+ label = "0.8V VCC";
+ reg = <2>;
+ };
+
+ input@3 {
+ label = "1.8V VCCIO_SDM";
+ reg = <3>;
+ };
+
+ input@4 {
+ label = "1.8V VCCPT";
+ reg = <4>;
+ };
+
+ input@5 {
+ label = "1.2V VCCCRCORE";
+ reg = <5>;
+ };
+
+ input@6 {
+ label = "0.9V VCCH";
+ reg = <6>;
+ };
+
+ input@7 {
+ label = "0.8V VCCL";
+ reg = <7>;
+ };
+ };
+
+ temperature {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ input@0 {
+ label = "Main Die SDM";
+ reg = <0x0>;
+ };
+
+ input@10000 {
+ label = "Main Die corner bottom left max";
+ reg = <0x10000>;
+ };
+
+ input@20000 {
+ label = "Main Die corner top left max";
+ reg = <0x20000>;
+ };
+
+ input@30000 {
+ label = "Main Die corner bottom right max";
+ reg = <0x30000>;
+ };
+
+ input@40000 {
+ label = "Main Die corner top right max";
+ reg = <0x40000>;
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts b/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts
index 08c088571270..70b9f0e56cc5 100644
--- a/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts
+++ b/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts
@@ -129,3 +129,49 @@ &usb0 {
&watchdog0 {
status = "okay";
};
+
+&temp_volt {
+ voltage {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ input@2 {
+ label = "0.8V VDD";
+ reg = <2>;
+ };
+
+ input@3 {
+ label = "0.8V VDD_SDM";
+ reg = <3>;
+ };
+
+ input@4 {
+ label = "1.8V VCCADC";
+ reg = <4>;
+ };
+
+ input@5 {
+ label = "1.8V VCCPD";
+ reg = <5>;
+ };
+
+ input@6 {
+ label = "1.8V VCCIO_SDM";
+ reg = <6>;
+ };
+
+ input@7 {
+ label = "0.8V VDD_HPS";
+ reg = <7>;
+ };
+ };
+
+ temperature {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ input@0 {
+ label = "Main Die SDM";
+ reg = <0x0>;
+ };
+ };
+};
--
2.40.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA
2023-04-10 15:33 ` [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA dinh.nguyen
@ 2023-04-10 23:48 ` Rob Herring
2023-04-11 6:53 ` Krzysztof Kozlowski
1 sibling, 0 replies; 14+ messages in thread
From: Rob Herring @ 2023-04-10 23:48 UTC (permalink / raw)
To: dinh.nguyen
Cc: dinguyen, robh+dt, linux, jdelvare, linux-kernel, devicetree,
krzysztof.kozlowski+dt, linux-hwmon
On Mon, 10 Apr 2023 10:33:11 -0500, dinh.nguyen@linux.intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>
> Document the hardware monitoring bindings for SoCFPGA 64-bit platforms.
>
> Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
> ---
> .../bindings/hwmon/intel,socfpga-hwmon.yaml | 241 ++++++++++++++++++
> 1 file changed, 241 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.example.dtb: temp_volt: 'reg' is a required property
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230410153314.27127-2-dinh.nguyen@linux.intel.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-10 15:33 ` [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms dinh.nguyen
@ 2023-04-11 2:44 ` Guenter Roeck
2023-04-17 20:55 ` Dinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2023-04-11 2:44 UTC (permalink / raw)
To: dinh.nguyen, linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, Andy Shevchenko
On 4/10/23 08:33, dinh.nguyen@linux.intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>
> The driver supports 64-bit SoCFPGA platforms for temperature and voltage
> reading using the platform's SDM(Secure Device Manager). The driver
> also uses the Stratix10 Service layer driver.
>
> This driver only supports OF SoCFPGA 64-bit platforms.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/socfpga-hwmon.rst | 30 ++
> drivers/firmware/stratix10-svc.c | 18 +-
Changes outside the hwmon subsystem need to be in a separate patch.
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/socfpga-hwmon.c | 406 ++++++++++++++++++
> include/linux/firmware/intel/stratix10-smc.h | 34 ++
> .../firmware/intel/stratix10-svc-client.h | 6 +
> 8 files changed, 506 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
> create mode 100644 drivers/hwmon/socfpga-hwmon.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index f1fe75f596a5..9db4e1537481 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -196,6 +196,7 @@ Hardware Monitoring Kernel Drivers
> smsc47b397
> smsc47m192
> smsc47m1
> + socfpga-hwmon
> sparx5-temp
> stpddc60
> sy7636a-hwmon
> diff --git a/Documentation/hwmon/socfpga-hwmon.rst b/Documentation/hwmon/socfpga-hwmon.rst
> new file mode 100644
> index 000000000000..f6565c83cf40
> --- /dev/null
> +++ b/Documentation/hwmon/socfpga-hwmon.rst
> @@ -0,0 +1,30 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +Kernel driver socfpga-hwmon
> +===========================
> +
> +Supported chips:
> +
> + * Intel Stratix10
> + * Intel Agilex
> + * Intel N5X
> +
> +Author: Dinh Nguyen <dinh.nguyen@linux.intel.com>
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for 64-Bit SoCFPGA and eASIC devices
> +based around the Secure Device Manager and Stratix 10 Service layer.
> +
> +The following sensor types are supported:
> +
> + * temperature
> + * voltage
> +
> +Usage Notes
> +-----------
> +
> +The driver relies on a device tree node to enumerate support present on the
> +specific device. See Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
> +for details of the device-tree node.
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index bde1f543f529..cc1b8b441c37 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -340,6 +340,8 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
> case COMMAND_RSU_MAX_RETRY:
> case COMMAND_RSU_DCMF_STATUS:
> case COMMAND_FIRMWARE_VERSION:
> + case COMMAND_HWMON_READTEMP:
> + case COMMAND_HWMON_READVOLT:
> cb_data->status = BIT(SVC_STATUS_OK);
> cb_data->kaddr1 = &res.a1;
> break;
> @@ -517,7 +519,17 @@ static int svc_normal_to_secure_thread(void *data)
> a1 = (unsigned long)pdata->paddr;
> a2 = 0;
> break;
> -
> + /* for HWMON */
> + case COMMAND_HWMON_READTEMP:
> + a0 = INTEL_SIP_SMC_HWMON_READTEMP;
> + a1 = pdata->arg[0];
> + a2 = 0;
> + break;
> + case COMMAND_HWMON_READVOLT:
> + a0 = INTEL_SIP_SMC_HWMON_READVOLT;
> + a1 = pdata->arg[0];
> + a2 = 0;
> + break;
> /* for polling */
> case COMMAND_POLL_SERVICE_STATUS:
> a0 = INTEL_SIP_SMC_SERVICE_COMPLETED;
> @@ -1182,6 +1194,10 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
> chans[2].name = SVC_CLIENT_FCS;
> spin_lock_init(&chans[2].lock);
>
> + chans[3].ctrl = controller;
> + chans[3].name = SVC_CLIENT_HWMON;
> + spin_lock_init(&chans[3].lock);
> +
> list_add_tail(&controller->node, &svc_ctrl);
> platform_set_drvdata(pdev, controller);
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5b3b76477b0e..c7c978acfece 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1875,6 +1875,17 @@ config SENSORS_SMSC47M192
> This driver can also be built as a module. If so, the module
> will be called smsc47m192.
>
> +config SENSORS_SOCFPGA
> + tristate "SoCFPGA Hardware monitoring features"
> + depends on INTEL_STRATIX10_SERVICE
> + depends on OF || COMPILE_TEST
> + help
> + If you say yes here you get support for the temperature and
> + voltage sensors of 64-bit SoCFPGA devices.
> +
> + This driver can also be built as a module. If so, the module
> + will be called socfpga-hwmon.
> +
> config SENSORS_SMSC47B397
> tristate "SMSC LPC47B397-NC"
> depends on !PPC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 88712b5031c8..c04c0b2578a4 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -193,6 +193,7 @@ obj-$(CONFIG_SENSORS_SMPRO) += smpro-hwmon.o
> obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
> obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> +obj-$(CONFIG_SENSORS_SOCFPGA) += socfpga-hwmon.o
> obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
> obj-$(CONFIG_SENSORS_STTS751) += stts751.o
> obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
> diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c
> new file mode 100644
> index 000000000000..636e6e269578
> --- /dev/null
> +++ b/drivers/hwmon/socfpga-hwmon.c
> @@ -0,0 +1,406 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SoCFPGA hardware monitoring features
> + *
> + * Copyright (c) 2023 Intel Corporation. All rights reserved
> + */
> +#include <linux/arm-smccc.h>
> +#include <linux/hwmon.h>
> +#include <linux/firmware/intel/stratix10-svc-client.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/units.h>
> +
> +#define HWMON_TIMEOUT msecs_to_jiffies(SVC_HWMON_REQUEST_TIMEOUT_MS)
> +
> +/*
> + * When bit 31 is set, an error condition has been detected in the
> + * temperature sensor.
> + */
> +#define ETEMP_ERROR BIT(31)
> +/*
> + * Selected temperature sensor channel is currently inactive.
> + * Ensure that the tile where the TSD is located is actively in use.
> + */
> +#define ETEMP_INACTIVE 0x0
> +/*
> + * Selected temperature sensor channel returned a value that is not the
> + * latest reading. Try retrieve the temperature reading again.
> + */
> +#define ETEMP_TOO_OLD 0x1
> +/*
> + * Selected temperature sensor channel is invalid for the device. Ignore
> + * the returned data because the temperature sensor channel location is
> + * invalid.
> + */
> +/* > + * System is corrupted or failed to respond.
> + */
> +#define ETEMP_TIMEOUT 0x3
> +#define ETEMP_CORRUPT 0x4
> +/*
> + * Communication mechanism is busy.
> + */
> +#define ETEMP_BUSY 0x5
> +/*
> + * System is corrupted or failed to respond.
> + */
> +#define ETEMP_NOT_INITIALIZED 0xFF
> +
> +#define SOCFPGA_HWMON_MAXSENSORS 16
> +
> +/**
> + * struct socfpga_hwmon_chan - channel input parameters.
> + * @n : Number of channels.
> + * @value: value read from the chip.
> + * @names: names array from DTS labels.
> + * @chan: channel array.
> + *
> + * The structure represents either the voltage or temperature information
> + * for the hwmon channels.
> + */
> +struct socfpga_hwmon_chan {
> + unsigned int n;
> + int value;
> + const char *names[SOCFPGA_HWMON_MAXSENSORS];
> + u32 chan[SOCFPGA_HWMON_MAXSENSORS];
> +};
> +
> +struct socfpga_hwmon_priv {
> + struct stratix10_svc_client client;
> + struct stratix10_svc_client_msg msg;
> + struct stratix10_svc_chan *chan;
> + struct completion completion;
> + struct mutex lock;
> + struct socfpga_hwmon_chan temperature;
> + struct socfpga_hwmon_chan voltage;
> +};
> +
> +enum hwmon_type_op {
> + SOCFPGA_HWMON_TYPE_TEMP,
> + SOCFPGA_HWMON_TYPE_VOLT,
> + SOCFPGA_HWMON_TYPE_MAX
Unused define
> +};
> +
> +static const char *const hwmon_types_str[] = { "temperature", "voltage" };
> +
> +static umode_t socfpga_is_visible(const void *dev,
> + enum hwmon_sensor_types type,
> + u32 attr, int chan)
> +{
> + switch (type) {
> + case hwmon_temp:
> + case hwmon_in:
> + return 0444;
> + default:
> + return 0;
> + }
> +}
> +
> +static void socfpga_smc_callback(struct stratix10_svc_client *client,
> + struct stratix10_svc_cb_data *data)
> +{
> + struct socfpga_hwmon_priv *priv = client->priv;
> + struct arm_smccc_res *res = data->kaddr1;
> +
> + if (data->status == BIT(SVC_STATUS_OK)) {
> + if (priv->msg.command == COMMAND_HWMON_READTEMP)
> + priv->temperature.value = res->a0;
> + else
> + priv->voltage.value = res->a0;
> + } else
> + dev_err(client->dev, "%s returned 0x%lX\n", __func__, res->a0);
> +
Missing { } in else branch. Please run checkpatch --strict and fix
continuation line alignment issues as well as unbalanced if/else
reports.
> + complete(&priv->completion);
> +}
> +
> +static int socfpga_hwmon_send(struct socfpga_hwmon_priv *priv)
> +{
> + int ret;
> +
> + priv->client.receive_cb = socfpga_smc_callback;
> +
> + ret = stratix10_svc_send(priv->chan, &priv->msg);
> + if (ret < 0)
> + return ret;
> +
> + if (!wait_for_completion_timeout(&priv->completion, HWMON_TIMEOUT)) {
> + dev_err(priv->client.dev, "SMC call timeout!\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
> +{
> + int value = priv->temperature.value;
> +
> + if (!(value & ETEMP_ERROR))
> + return 0;
> +
This is odd. int is normally 32 bit, this function is called from
socfpga_read() for temperatures, which presumably are defined
as "signed 32-bit fixed point binary". That means that negative
temperatures would be treated as errors. Please verify.
> + dev_err(priv->client.dev, "temperature sensor code 0x%08x\n", value);
> +
Please don't clog the log with such messages.
> + value &= ~ETEMP_ERROR;
> + switch (value) {
> + case ETEMP_NOT_PRESENT:
> + return -ENOENT;
> + case ETEMP_CORRUPT:
> + case ETEMP_NOT_INITIALIZED:
> + return -ENODATA;
> + case ETEMP_BUSY:
> + return -EBUSY;
> + case ETEMP_INACTIVE:
> + case ETEMP_TIMEOUT:
> + case ETEMP_TOO_OLD:
> + return -EAGAIN;
> + default:
> + /* Unknown error */
> + return -EINVAL;
Should be -EIO.
> + }
> +}
> +
> +static int socfpga_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int chan, long *val)
> +{
> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&priv->lock);
> + reinit_completion(&priv->completion);
> +
> + switch (type) {
> + case hwmon_temp:
> + priv->msg.arg[0] = BIT_ULL(priv->temperature.chan[chan]);
> + priv->msg.command = COMMAND_HWMON_READTEMP;
> + if (socfpga_hwmon_send(priv))
> + goto status_done;
> +
> + ret = socfpga_hwmon_err_to_errno(priv);
> + if (ret)
> + break;
> + /*
> + * The Temperature Sensor IP core returns the Celsius
> + * temperature value in signed 32-bit fixed point binary
> + * format, with eight bits below binary point.
> + */
> + *val = (priv->temperature.value * MILLIDEGREE_PER_DEGREE) / 256;
> + break;
> + case hwmon_in: /* Read voltage */
Pointless comment
> + priv->msg.arg[0] = BIT_ULL(priv->voltage.chan[chan]);
> + priv->msg.command = COMMAND_HWMON_READVOLT;
> + if (socfpga_hwmon_send(priv))
> + goto status_done;
> +
No error check for voltage sensors ?
Also, unless I am missing something, the error bailout leaves ret
undefined.
> + /*
> + * The Voltage Sensor IP core returns the sampled voltage
> + * in unsigned 32-bit fixed point binary format, with 16 bits
> + * below binary point.
> + */
> + *val = (priv->voltage.value * MILLIVOLT_PER_VOLT) / 65536;
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> +status_done:
> + stratix10_svc_done(priv->chan);
> + mutex_unlock(&priv->lock);
> + return ret;
> +}
> +
> +static int socfpga_read_string(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int chan, const char **str)
> +{
> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_in:
> + *str = priv->voltage.names[chan];
> + return 0;
> + case hwmon_temp:
> + *str = priv->temperature.names[chan];
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static const struct hwmon_ops socfpga_ops = {
> + .is_visible = socfpga_is_visible,
> + .read = socfpga_read,
> + .read_string = socfpga_read_string,
> +};
> +
> +static const struct hwmon_channel_info *socfpga_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL),
> + HWMON_CHANNEL_INFO(in,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL),
> + NULL
> +};
> +
> +static const struct hwmon_chip_info socfpga_chip_info = {
> + .ops = &socfpga_ops,
> + .info = socfpga_info,
> +};
> +
> +static int socfpga_add_channel(struct device *dev, const char *type,
> + u32 val, const char *label,
> + struct socfpga_hwmon_priv *priv)
> +{
> + int type_index;
> + struct socfpga_hwmon_chan *p;
> +
> + type_index = match_string(hwmon_types_str, ARRAY_SIZE(hwmon_types_str), type);
> + switch (type_index) {
> + case SOCFPGA_HWMON_TYPE_TEMP:
> + p = &priv->temperature;
> + break;
> + case SOCFPGA_HWMON_TYPE_VOLT:
> + p = &priv->voltage;
> + break;
> + default:
> + return -ENODATA;
> + }
> + if (p->n >= SOCFPGA_HWMON_MAXSENSORS)
> + return -ENOSPC;
> +
> + p->names[p->n] = label;
> + p->chan[p->n] = val;
> + p->n++;
> +
> + return 0;
> +}
> +
> +static int socfpga_probe_child_from_dt(struct device *dev,
> + struct device_node *child,
> + struct socfpga_hwmon_priv *priv)
> +{
> + struct device_node *grandchild;
> + const char *label;
> + const char *type;
> + u32 val;
> + int ret;
> +
> + if (of_property_read_string(child, "name", &type))
> + return dev_err_probe(dev, -EINVAL, "No type for %pOF\n", child);
> +
> + for_each_child_of_node(child, grandchild) {
> + ret = of_property_read_u32(grandchild, "reg", &val);
> + if (ret)
> + return dev_err_probe(dev, ret, "missing reg property of %pOF\n",
> + grandchild);
> +
> + ret = of_property_read_string(grandchild, "label", &label);
> + if (ret)
> + return dev_err_probe(dev, ret, "missing label propoerty of %pOF\n",
> + grandchild);
> + ret = socfpga_add_channel(dev, type, val, label, priv);
> + if (ret == -ENOSPC)
> + return dev_err_probe(dev, ret, "too many channels, only %d supported\n",
> + SOCFPGA_HWMON_MAXSENSORS);
> + }
> + return 0;
> +}
> +
> +static int socfpga_probe_from_dt(struct device *dev,
> + struct socfpga_hwmon_priv *priv)
> +{
> + const struct device_node *np = dev->of_node;
> + struct device_node *child;
> + int ret = 0;
> +
> + for_each_child_of_node(np, child) {
> + ret = socfpga_probe_child_from_dt(dev, child, priv);
> + if (ret)
> + break;
> + }
> + of_node_put(child);
> +
> + return ret;
> +}
> +
> +static int socfpga_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device *hwmon_dev;
> + struct socfpga_hwmon_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->client.dev = dev;
> + priv->client.priv = priv;
> +
> + ret = socfpga_probe_from_dt(dev, priv);
> + if (ret)
> + return dev_err_probe(dev, ret, "Unable to probe from device tree\n");
> +
> + mutex_init(&priv->lock);
> + init_completion(&priv->completion);
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpgahwmon",
> + priv,
> + &socfpga_chip_info,
> + NULL);
> + if (IS_ERR(hwmon_dev))
> + return PTR_ERR(hwmon_dev);
> +
> + priv->chan = stratix10_svc_request_channel_byname(&priv->client,
> + SVC_CLIENT_HWMON);
This is racy: hwmon attributes exist here, and priv->chan may be accessed before
it is set.
> + if (IS_ERR(priv->chan))
> + return dev_err_probe(dev, PTR_ERR(priv->chan),
> + "couldn't get service channel %s\n",
> + SVC_CLIENT_RSU);
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int socfpga_hwmon_remove(struct platform_device *pdev)
> +{
> + struct socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
> +
> + stratix10_svc_free_channel(priv->chan);
This releases the channel before the hwmon device is released.
Another race condition.
> + return 0;
> +}
> +
> +static const struct of_device_id socfpga_of_match[] = {
> + { .compatible = "intel,socfpga-hwmon" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, socfpga_of_match);
> +
> +static struct platform_driver socfpga_hwmon_driver = {
> + .driver = {
> + .name = "socfpga-hwmon",
> + .of_match_table = socfpga_of_match,
> + },
> + .probe = socfpga_hwmon_probe,
> + .remove = socfpga_hwmon_remove,
> +};
> +module_platform_driver(socfpga_hwmon_driver);
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("SoCFPGA hardware monitoring features");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/linux/firmware/intel/stratix10-smc.h
> index a718f853d457..b944ec4b2b2f 100644
> --- a/include/linux/firmware/intel/stratix10-smc.h
> +++ b/include/linux/firmware/intel/stratix10-smc.h
> @@ -595,4 +595,38 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
> #define INTEL_SIP_SMC_FCS_GET_PROVISION_DATA \
> INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FCS_GET_PROVISION_DATA)
>
> +/**
> + * Request INTEL_SIP_SMC_HWMON_READTEMP
> + * Sync call to request temperature
> + *
> + * Call register usage:
> + * a0 Temperature Channel
> + * a1-a7 not used
> + *
> + * Return status
> + * a0 INTEL_SIP_SMC_STATUS_OK
> + * a1 Temperature Value
> + * a2-a3 not used
> + */
> +#define INTEL_SIP_SMC_FUNCID_HWMON_READTEMP 32
> +#define INTEL_SIP_SMC_HWMON_READTEMP \
> + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_HWMON_READTEMP)
> +
> +/**
> + * Request INTEL_SIP_SMC_HWMON_READVOLT
> + * Sync call to request voltage
> + *
> + * Call register usage:
> + * a0 Voltage Channel
> + * a1-a7 not used
> + *
> + * Return status
> + * a0 INTEL_SIP_SMC_STATUS_OK
> + * a1 Voltage Value
> + * a2-a3 not used
> + */
> +#define INTEL_SIP_SMC_FUNCID_HWMON_READVOLT 33
> +#define INTEL_SIP_SMC_HWMON_READVOLT \
> + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_HWMON_READVOLT)
> +
> #endif
> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> index 0c16037fd08d..343970dcc2d2 100644
> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> @@ -11,10 +11,12 @@
> *
> * fpga: for FPGA configuration
> * rsu: for remote status update
> + * hwmon: for hardware monitoring (voltage and temperature)
> */
> #define SVC_CLIENT_FPGA "fpga"
> #define SVC_CLIENT_RSU "rsu"
> #define SVC_CLIENT_FCS "fcs"
> +#define SVC_CLIENT_HWMON "hwmon"
>
> /*
> * Status of the sent command, in bit number
> @@ -70,6 +72,7 @@
> #define SVC_RSU_REQUEST_TIMEOUT_MS 300
> #define SVC_FCS_REQUEST_TIMEOUT_MS 2000
> #define SVC_COMPLETED_TIMEOUT_MS 30000
> +#define SVC_HWMON_REQUEST_TIMEOUT_MS 300
>
> struct stratix10_svc_chan;
>
> @@ -164,6 +167,9 @@ enum stratix10_svc_command_code {
> COMMAND_FCS_RANDOM_NUMBER_GEN,
> /* for general status poll */
> COMMAND_POLL_SERVICE_STATUS = 40,
> + /* for HWMON */
> + COMMAND_HWMON_READTEMP,
> + COMMAND_HWMON_READVOLT,
> /* Non-mailbox SMC Call */
> COMMAND_SMC_SVC_VERSION = 200,
> };
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA
2023-04-10 15:33 ` [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA dinh.nguyen
2023-04-10 23:48 ` Rob Herring
@ 2023-04-11 6:53 ` Krzysztof Kozlowski
1 sibling, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-11 6:53 UTC (permalink / raw)
To: dinh.nguyen, linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, linux
On 10/04/2023 17:33, dinh.nguyen@linux.intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>
> Document the hardware monitoring bindings for SoCFPGA 64-bit platforms.
>
> Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
> ---
> .../bindings/hwmon/intel,socfpga-hwmon.yaml | 241 ++++++++++++++++++
> 1 file changed, 241 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
> new file mode 100644
> index 000000000000..ec9d9eabdc37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
> @@ -0,0 +1,241 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/intel,socfpga-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel SoCFPGA Hardware monitor
> +
> +maintainers:
> + - Dinh Nguyen <dinh.nguyen@linux.intel.com>
> +
> +description: |
> + The Intel SoCFPGA hardware monitor unit provides on-chip voltage and
> + temperature sensors. You can use these sensors to monitor external
> + voltages and on-chip operating conditions such as internal power rails
> + and on-chip junction temperatures.
> +
> + The specific sensor configurations vary for each device family and
> + each device within a family does not offer all potential sensor
> + options. The information below attempts to illustrate the super set of
> + possible sensor options that are potentially available within each
> + device family, but the user should check the documentation for the
> + specific device they are using to verify which sensor options it
> + actually provides.
> +
> + Stratix 10 Device Family
> +
> + Stratix 10 Voltage Sensors
> +
> + page 0, channel 2 = 0.8V VCC
> + page 0, channel 3 = 1.0V VCCIO
> + page 0, channel 6 = 0.9V VCCERAM
> +
> + Stratix 10 Temperature Sensors
> +
> + page 0, channel 0 = main die
> + page 0, channel 1 = tile bottom left
> + page 0, channel 2 = tile middle left
> + page 0, channel 3 = tile top left
> + page 0, channel 4 = tile bottom right
> + page 0, channel 5 = tile middle right
> + page 0, channel 6 = tile top right
> + page 0, channel 7 = hbm2 bottom
> + page 0, channel 8 = hbm2 top
> +
> + Agilex Device Family
> +
> + Agilex Voltage Sensors
> +
> + page 0, channel 2 = 0.8V VCC
> + page 0, channel 3 = 1.8V VCCIO_SDM
> + page 0, channel 4 = 1.8V VCCPT
> + page 0, channel 5 = 1.2V VCCRCORE
> + page 0, channel 6 = 0.9V VCCH
> + page 0, channel 7 = 0.8V VCCL
> +
> + Agilex Temperature Sensors
> +
> + page 0, channel 0 = main die sdm max
> + page 0, channel 1 = main die sdm 1
> +
> + page 1, channel 0 = main die corner bottom left max
> + page 1, channel 1 = main die corner bottom left 1
> + page 1, channel 2 = main die corner bottom left 2
> +
> + page 2, channel 0 = main die corner top left max
> + page 2, channel 1 = main die corner top left 1
> + page 2, channel 2 = main die corner top left 2
> +
> + page 3, channel 0 = main die corner bottom right max
> + page 3, channel 1 = main die corner bottom right 1
> + page 3, channel 2 = main die corner bottom right 2
> +
> + page 4, channel 0 = main die corner top right max
> + page 4, channel 1 = main die corner top right 1
> + page 4, channel 2 = main die corner top right 2
> +
> + page 5, channel 0 = tile die bottom left max
> + page 5, channel 1 = tile die bottom left 1
> + page 5, channel 6..2 = tile die bottom left 6..2 R-tile only
> + page 5, channel 5..2 = tile die bottom left 5..2 F-tile only
> + page 5, channel 4..2 = tile die bottom left 4..2 E-tile only
> +
> + page 7, channel 0 = tile die top left max
> + page 7, channel 1 = tile die top left 1
> + page 7, channel 6..2 = tile die top left 6..2 R-tile only
> + page 7, channel 5..2 = tile die top left 5..2 F-tile only
> + page 7, channel 4..2 = tile die top left 4..2 E-tile only
> +
> + page 8, channel 0 = tile die bottom right max
> + page 8, channel 1 = tile die bottom right 1
> + page 8, channel 6..2 = tile die bottom right 6..2 R-tile only
> + page 8, channel 5..2 = tile die bottom right 5..2 F-tile only
> + page 8, channel 4..2 = tile die bottom right 4..2 E-tile only
> +
> + page 10, channel 0 = tile die top right max
> + page 10, channel 1 = tile die top right 1
> + page 10, channel 6..2 = tile die top right 6..2 R-tile only
> + page 10, channel 5..2 = tile die top right 5..2 F-tile only
> + page 10, channel 4..2 = tile die top right 4..2 E-tile only
> +
> + N5X Device Family
> +
> + N5X Voltage Sensors
> +
> + page 0, channel 2 = 0.8V VDD
> + page 0, channel 3 = 0.8V VDD_SDM
> + page 0, channel 4 = 1.8V VCCADC
> + page 0, channel 5 = 1.8V VCCPD
> + page 0, channel 6 = 1.8V VCCIO_SDM
> + page 0, channel 7 = 0.8V VDD_HPS
> +
> + N5X Temperature Sensors
> +
> + page 0, channel 0 = main die
> +
> +properties:
> + compatible:
> + enum:
> + - intel,socfpga-hwmon
You should have SoC specific compatibles, followed by one specific or by
generic.
> +
> + reg:
> + maxItems: 1
> + description:
> + The sensor mapping address is denoted by the lower 16-bits being
> + the channel mask location that defines the channel number.
> + The upper 16-bits denotes the page number.
> + The bit mask of 0x1 represents channel 1. The supported
> + page and channel is dependent on the SoCFPGA variant.
> + Page number greater than 0 is only supported on the
> + temperature sensors.
> +
> + temperature:
> + description:
> + Specifies the possible mappings of temperature sensors
> + diodes on the SoCFPGA main die and tile die.
What's this? No ref/type, not constraints?
> +
> + voltage:
> + description:
> + Specifies the possible mappings of the voltage sensors on the
> + SoCFPGA analog to digital converter of the Secure Device Manager
> + (SDM).
Same here.
> +
> + input:
> + description:
> + Specifies each sensor.
And here.
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + temp_volt {
No underscores in node names.
> + compatible = "intel,socfpga-hwmon";
> + voltage {
> + #address-cells = <1>;
> + #size-cells = <0>;
So this is object?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-11 2:44 ` Guenter Roeck
@ 2023-04-17 20:55 ` Dinh Nguyen
2023-04-17 21:51 ` Guenter Roeck
0 siblings, 1 reply; 14+ messages in thread
From: Dinh Nguyen @ 2023-04-17 20:55 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, Andy Shevchenko
On 4/10/2023 9:44 PM, Guenter Roeck wrote:
> On 4/10/23 08:33, dinh.nguyen@linux.intel.com wrote:
>> From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>>
>> The driver supports 64-bit SoCFPGA platforms for temperature and voltage
>> reading using the platform's SDM(Secure Device Manager). The driver
>> also uses the Stratix10 Service layer driver.
>>
>> This driver only supports OF SoCFPGA 64-bit platforms.
>>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>> ---
>> Documentation/hwmon/index.rst | 1 +
>> Documentation/hwmon/socfpga-hwmon.rst | 30 ++
>> drivers/firmware/stratix10-svc.c | 18 +-
>
> Changes outside the hwmon subsystem need to be in a separate patch.
will separate...
>
>> drivers/hwmon/Kconfig | 11 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/socfpga-hwmon.c | 406 ++++++++++++++++++
>> include/linux/firmware/intel/stratix10-smc.h | 34 ++
>> .../firmware/intel/stratix10-svc-client.h | 6 +
>> 8 files changed, 506 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
>> create mode 100644 drivers/hwmon/socfpga-hwmon.c
>>
>>
...
>> +
>> +enum hwmon_type_op {
>> + SOCFPGA_HWMON_TYPE_TEMP,
>> + SOCFPGA_HWMON_TYPE_VOLT,
>> + SOCFPGA_HWMON_TYPE_MAX
>
> Unused define
Removed.
>
>> +};
>> +
>> +static const char *const hwmon_types_str[] = { "temperature",
>> "voltage" };
>> +
>> +static umode_t socfpga_is_visible(const void *dev,
>> + enum hwmon_sensor_types type,
>> + u32 attr, int chan)
>> +{
>> + switch (type) {
>> + case hwmon_temp:
>> + case hwmon_in:
>> + return 0444;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +static void socfpga_smc_callback(struct stratix10_svc_client *client,
>> + struct stratix10_svc_cb_data *data)
>> +{
>> + struct socfpga_hwmon_priv *priv = client->priv;
>> + struct arm_smccc_res *res = data->kaddr1;
>> +
>> + if (data->status == BIT(SVC_STATUS_OK)) {
>> + if (priv->msg.command == COMMAND_HWMON_READTEMP)
>> + priv->temperature.value = res->a0;
>> + else
>> + priv->voltage.value = res->a0;
>> + } else
>> + dev_err(client->dev, "%s returned 0x%lX\n", __func__, res->a0);
>> +
>
> Missing { } in else branch. Please run checkpatch --strict and fix
> continuation line alignment issues as well as unbalanced if/else
> reports.
Will do.
>
>> + complete(&priv->completion);
>> +}
>> +
>> +static int socfpga_hwmon_send(struct socfpga_hwmon_priv *priv)
>> +{
>> + int ret;
>> +
>> + priv->client.receive_cb = socfpga_smc_callback;
>> +
>> + ret = stratix10_svc_send(priv->chan, &priv->msg);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (!wait_for_completion_timeout(&priv->completion,
>> HWMON_TIMEOUT)) {
>> + dev_err(priv->client.dev, "SMC call timeout!\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
>> +{
>> + int value = priv->temperature.value;
>> +
>> + if (!(value & ETEMP_ERROR))
>> + return 0;
>> +
>
> This is odd. int is normally 32 bit, this function is called from
> socfpga_read() for temperatures, which presumably are defined
> as "signed 32-bit fixed point binary". That means that negative
> temperatures would be treated as errors. Please verify.
That's correct, if bit 31 is set, then it indicates an error.
>
>> + dev_err(priv->client.dev, "temperature sensor code 0x%08x\n",
>> value);
>> +
>
> Please don't clog the log with such messages.
Removed.
>
>> + value &= ~ETEMP_ERROR;
>> + switch (value) {
>> + case ETEMP_NOT_PRESENT:
>> + return -ENOENT;
>> + case ETEMP_CORRUPT:
>> + case ETEMP_NOT_INITIALIZED:
>> + return -ENODATA;
>> + case ETEMP_BUSY:
>> + return -EBUSY;
>> + case ETEMP_INACTIVE:
>> + case ETEMP_TIMEOUT:
>> + case ETEMP_TOO_OLD:
>> + return -EAGAIN;
>> + default:
>> + /* Unknown error */
>> + return -EINVAL;
>
> Should be -EIO.
>
Replaced.
>> + }
>> +}
>> +
>> +static int socfpga_read(struct device *dev, enum hwmon_sensor_types
>> type,
>> + u32 attr, int chan, long *val)
>> +{
>> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + mutex_lock(&priv->lock);
>> + reinit_completion(&priv->completion);
>> +
>> + switch (type) {
>> + case hwmon_temp:
>> + priv->msg.arg[0] = BIT_ULL(priv->temperature.chan[chan]);
>> + priv->msg.command = COMMAND_HWMON_READTEMP;
>> + if (socfpga_hwmon_send(priv))
>> + goto status_done;
>> +
>> + ret = socfpga_hwmon_err_to_errno(priv);
>> + if (ret)
>> + break;
>> + /*
>> + * The Temperature Sensor IP core returns the Celsius
>> + * temperature value in signed 32-bit fixed point binary
>> + * format, with eight bits below binary point.
>> + */
>> + *val = (priv->temperature.value * MILLIDEGREE_PER_DEGREE) /
>> 256;
>> + break;
>> + case hwmon_in: /* Read voltage */
>
> Pointless comment
Removed.
>
>> + priv->msg.arg[0] = BIT_ULL(priv->voltage.chan[chan]);
>> + priv->msg.command = COMMAND_HWMON_READVOLT;
>> + if (socfpga_hwmon_send(priv))
>> + goto status_done;
>> +
>
> No error check for voltage sensors ?
> Also, unless I am missing something, the error bailout leaves ret
> undefined.
>
There are no error readings for the voltage sensors specified in the
spec. Will update
the ret value.
>> + /*
>> + * The Voltage Sensor IP core returns the sampled voltage
>> + * in unsigned 32-bit fixed point binary format, with 16 bits
>> + * below binary point.
>> + */
>> + *val = (priv->voltage.value * MILLIVOLT_PER_VOLT) / 65536;
>> + break;
>> + default:
>> + ret = -EOPNOTSUPP;
>> + break;
>> + }
>> +
>> +status_done:
>> + stratix10_svc_done(priv->chan);
>> + mutex_unlock(&priv->lock);
>> + return ret;
>> +}
>> +
>> +static int socfpga_read_string(struct device *dev,
>> + enum hwmon_sensor_types type, u32 attr,
>> + int chan, const char **str)
>> +{
>> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
>> +
>> + switch (type) {
>> + case hwmon_in:
>> + *str = priv->voltage.names[chan];
>> + return 0;
>> + case hwmon_temp:
>> + *str = priv->temperature.names[chan];
>> + return 0;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +static const struct hwmon_ops socfpga_ops = {
>> + .is_visible = socfpga_is_visible,
>> + .read = socfpga_read,
>> + .read_string = socfpga_read_string,
>> +};
>> +
>> +static const struct hwmon_channel_info *socfpga_info[] = {
>> + HWMON_CHANNEL_INFO(temp,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL),
>> + HWMON_CHANNEL_INFO(in,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL),
>> + NULL
>> +};
>> +
>> +static const struct hwmon_chip_info socfpga_chip_info = {
>> + .ops = &socfpga_ops,
>> + .info = socfpga_info,
>> +};
>> +
>> +static int socfpga_add_channel(struct device *dev, const char *type,
>> + u32 val, const char *label,
>> + struct socfpga_hwmon_priv *priv)
>> +{
>> + int type_index;
>> + struct socfpga_hwmon_chan *p;
>> +
>> + type_index = match_string(hwmon_types_str,
>> ARRAY_SIZE(hwmon_types_str), type);
>> + switch (type_index) {
>> + case SOCFPGA_HWMON_TYPE_TEMP:
>> + p = &priv->temperature;
>> + break;
>> + case SOCFPGA_HWMON_TYPE_VOLT:
>> + p = &priv->voltage;
>> + break;
>> + default:
>> + return -ENODATA;
>> + }
>> + if (p->n >= SOCFPGA_HWMON_MAXSENSORS)
>> + return -ENOSPC;
>> +
>> + p->names[p->n] = label;
>> + p->chan[p->n] = val;
>> + p->n++;
>> +
>> + return 0;
>> +}
>> +
>> +static int socfpga_probe_child_from_dt(struct device *dev,
>> + struct device_node *child,
>> + struct socfpga_hwmon_priv *priv)
>> +{
>> + struct device_node *grandchild;
>> + const char *label;
>> + const char *type;
>> + u32 val;
>> + int ret;
>> +
>> + if (of_property_read_string(child, "name", &type))
>> + return dev_err_probe(dev, -EINVAL, "No type for %pOF\n",
>> child);
>> +
>> + for_each_child_of_node(child, grandchild) {
>> + ret = of_property_read_u32(grandchild, "reg", &val);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "missing reg property of
>> %pOF\n",
>> + grandchild);
>> +
>> + ret = of_property_read_string(grandchild, "label", &label);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "missing label propoerty
>> of %pOF\n",
>> + grandchild);
>> + ret = socfpga_add_channel(dev, type, val, label, priv);
>> + if (ret == -ENOSPC)
>> + return dev_err_probe(dev, ret, "too many channels, only
>> %d supported\n",
>> + SOCFPGA_HWMON_MAXSENSORS);
>> + }
>> + return 0;
>> +}
>> +
>> +static int socfpga_probe_from_dt(struct device *dev,
>> + struct socfpga_hwmon_priv *priv)
>> +{
>> + const struct device_node *np = dev->of_node;
>> + struct device_node *child;
>> + int ret = 0;
>> +
>> + for_each_child_of_node(np, child) {
>> + ret = socfpga_probe_child_from_dt(dev, child, priv);
>> + if (ret)
>> + break;
>> + }
>> + of_node_put(child);
>> +
>> + return ret;
>> +}
>> +
>> +static int socfpga_hwmon_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device *hwmon_dev;
>> + struct socfpga_hwmon_priv *priv;
>> + int ret;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->client.dev = dev;
>> + priv->client.priv = priv;
>> +
>> + ret = socfpga_probe_from_dt(dev, priv);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Unable to probe from device
>> tree\n");
>> +
>> + mutex_init(&priv->lock);
>> + init_completion(&priv->completion);
>> + hwmon_dev = devm_hwmon_device_register_with_info(dev,
>> "socfpgahwmon",
>> + priv,
>> + &socfpga_chip_info,
>> + NULL);
>> + if (IS_ERR(hwmon_dev))
>> + return PTR_ERR(hwmon_dev);
>> +
>> + priv->chan = stratix10_svc_request_channel_byname(&priv->client,
>> + SVC_CLIENT_HWMON);
>
> This is racy: hwmon attributes exist here, and priv->chan may be
> accessed before
> it is set.
Fixed in v2.
>
>> + if (IS_ERR(priv->chan))
>> + return dev_err_probe(dev, PTR_ERR(priv->chan),
>> + "couldn't get service channel %s\n",
>> + SVC_CLIENT_RSU);
>> +
>> + platform_set_drvdata(pdev, priv);
>> +
>> + return 0;
>> +}
>> +
>> +static int socfpga_hwmon_remove(struct platform_device *pdev)
>> +{
>> + struct socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
>> +
>> + stratix10_svc_free_channel(priv->chan);
>
> This releases the channel before the hwmon device is released.
> Another race condition.
fixed in V2.
Thanks for the review!
Dinh
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-17 20:55 ` Dinh Nguyen
@ 2023-04-17 21:51 ` Guenter Roeck
2023-04-18 17:29 ` Dinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2023-04-17 21:51 UTC (permalink / raw)
To: Dinh Nguyen, linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, Andy Shevchenko
On 4/17/23 13:55, Dinh Nguyen wrote:
>
> On 4/10/2023 9:44 PM, Guenter Roeck wrote:
>> On 4/10/23 08:33, dinh.nguyen@linux.intel.com wrote:
>>> From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>>>
>>> The driver supports 64-bit SoCFPGA platforms for temperature and voltage
>>> reading using the platform's SDM(Secure Device Manager). The driver
>>> also uses the Stratix10 Service layer driver.
>>>
>>> This driver only supports OF SoCFPGA 64-bit platforms.
>>>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>>> ---
>>> Documentation/hwmon/index.rst | 1 +
>>> Documentation/hwmon/socfpga-hwmon.rst | 30 ++
>>> drivers/firmware/stratix10-svc.c | 18 +-
>>
>> Changes outside the hwmon subsystem need to be in a separate patch.
>
> will separate...
>
>>
>>> drivers/hwmon/Kconfig | 11 +
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/socfpga-hwmon.c | 406 ++++++++++++++++++
>>> include/linux/firmware/intel/stratix10-smc.h | 34 ++
>>> .../firmware/intel/stratix10-svc-client.h | 6 +
>>> 8 files changed, 506 insertions(+), 1 deletion(-)
>>> create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
>>> create mode 100644 drivers/hwmon/socfpga-hwmon.c
>>>
>>>
> ...
>>> +
>>> +enum hwmon_type_op {
>>> + SOCFPGA_HWMON_TYPE_TEMP,
>>> + SOCFPGA_HWMON_TYPE_VOLT,
>>> + SOCFPGA_HWMON_TYPE_MAX
>>
>> Unused define
>
> Removed.
>
>>
>>> +};
>>> +
>>> +static const char *const hwmon_types_str[] = { "temperature", "voltage" };
>>> +
>>> +static umode_t socfpga_is_visible(const void *dev,
>>> + enum hwmon_sensor_types type,
>>> + u32 attr, int chan)
>>> +{
>>> + switch (type) {
>>> + case hwmon_temp:
>>> + case hwmon_in:
>>> + return 0444;
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static void socfpga_smc_callback(struct stratix10_svc_client *client,
>>> + struct stratix10_svc_cb_data *data)
>>> +{
>>> + struct socfpga_hwmon_priv *priv = client->priv;
>>> + struct arm_smccc_res *res = data->kaddr1;
>>> +
>>> + if (data->status == BIT(SVC_STATUS_OK)) {
>>> + if (priv->msg.command == COMMAND_HWMON_READTEMP)
>>> + priv->temperature.value = res->a0;
>>> + else
>>> + priv->voltage.value = res->a0;
>>> + } else
>>> + dev_err(client->dev, "%s returned 0x%lX\n", __func__, res->a0);
>>> +
>>
>> Missing { } in else branch. Please run checkpatch --strict and fix
>> continuation line alignment issues as well as unbalanced if/else
>> reports.
> Will do.
>>
>>> + complete(&priv->completion);
>>> +}
>>> +
>>> +static int socfpga_hwmon_send(struct socfpga_hwmon_priv *priv)
>>> +{
>>> + int ret;
>>> +
>>> + priv->client.receive_cb = socfpga_smc_callback;
>>> +
>>> + ret = stratix10_svc_send(priv->chan, &priv->msg);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (!wait_for_completion_timeout(&priv->completion, HWMON_TIMEOUT)) {
>>> + dev_err(priv->client.dev, "SMC call timeout!\n");
>>> + return -ETIMEDOUT;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
>>> +{
>>> + int value = priv->temperature.value;
>>> +
>>> + if (!(value & ETEMP_ERROR))
>>> + return 0;
>>> +
>>
>> This is odd. int is normally 32 bit, this function is called from
>> socfpga_read() for temperatures, which presumably are defined
>> as "signed 32-bit fixed point binary". That means that negative
>> temperatures would be treated as errors. Please verify.
>
> That's correct, if bit 31 is set, then it indicates an error.
>
This ...
>>
>>> + dev_err(priv->client.dev, "temperature sensor code 0x%08x\n", value);
>>> +
>>
>> Please don't clog the log with such messages.
>
> Removed.
>
>>
>>> + value &= ~ETEMP_ERROR;
>>> + switch (value) {
>>> + case ETEMP_NOT_PRESENT:
>>> + return -ENOENT;
>>> + case ETEMP_CORRUPT:
>>> + case ETEMP_NOT_INITIALIZED:
>>> + return -ENODATA;
>>> + case ETEMP_BUSY:
>>> + return -EBUSY;
>>> + case ETEMP_INACTIVE:
>>> + case ETEMP_TIMEOUT:
>>> + case ETEMP_TOO_OLD:
>>> + return -EAGAIN;
>>> + default:
>>> + /* Unknown error */
>>> + return -EINVAL;
>>
>> Should be -EIO.
>>
> Replaced.
>>> + }
>>> +}
>>> +
>>> +static int socfpga_read(struct device *dev, enum hwmon_sensor_types type,
>>> + u32 attr, int chan, long *val)
>>> +{
>>> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
>>> + int ret;
>>> +
>>> + mutex_lock(&priv->lock);
>>> + reinit_completion(&priv->completion);
>>> +
>>> + switch (type) {
>>> + case hwmon_temp:
>>> + priv->msg.arg[0] = BIT_ULL(priv->temperature.chan[chan]);
>>> + priv->msg.command = COMMAND_HWMON_READTEMP;
>>> + if (socfpga_hwmon_send(priv))
>>> + goto status_done;
>>> +
>>> + ret = socfpga_hwmon_err_to_errno(priv);
>>> + if (ret)
>>> + break;
>>> + /*
>>> + * The Temperature Sensor IP core returns the Celsius
>>> + * temperature value in signed 32-bit fixed point binary
... and this contradict each other. If bit 31 indicates an error,
this can not be a signed 32-bit value.
Guenter
>>> + * format, with eight bits below binary point.
>>> + */
>>> + *val = (priv->temperature.value * MILLIDEGREE_PER_DEGREE) / 256;
>>> + break;
>>> + case hwmon_in: /* Read voltage */
>>
>> Pointless comment
>
> Removed.
>
>>
>>> + priv->msg.arg[0] = BIT_ULL(priv->voltage.chan[chan]);
>>> + priv->msg.command = COMMAND_HWMON_READVOLT;
>>> + if (socfpga_hwmon_send(priv))
>>> + goto status_done;
>>> +
>>
>> No error check for voltage sensors ?
>> Also, unless I am missing something, the error bailout leaves ret
>> undefined.
>>
> There are no error readings for the voltage sensors specified in the spec. Will update
>
> the ret value.
>
>>> + /*
>>> + * The Voltage Sensor IP core returns the sampled voltage
>>> + * in unsigned 32-bit fixed point binary format, with 16 bits
>>> + * below binary point.
>>> + */
>>> + *val = (priv->voltage.value * MILLIVOLT_PER_VOLT) / 65536;
>>> + break;
>>> + default:
>>> + ret = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> +
>>> +status_done:
>>> + stratix10_svc_done(priv->chan);
>>> + mutex_unlock(&priv->lock);
>>> + return ret;
>>> +}
>>> +
>>> +static int socfpga_read_string(struct device *dev,
>>> + enum hwmon_sensor_types type, u32 attr,
>>> + int chan, const char **str)
>>> +{
>>> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
>>> +
>>> + switch (type) {
>>> + case hwmon_in:
>>> + *str = priv->voltage.names[chan];
>>> + return 0;
>>> + case hwmon_temp:
>>> + *str = priv->temperature.names[chan];
>>> + return 0;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +}
>>> +
>>> +static const struct hwmon_ops socfpga_ops = {
>>> + .is_visible = socfpga_is_visible,
>>> + .read = socfpga_read,
>>> + .read_string = socfpga_read_string,
>>> +};
>>> +
>>> +static const struct hwmon_channel_info *socfpga_info[] = {
>>> + HWMON_CHANNEL_INFO(temp,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
>>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL),
>>> + HWMON_CHANNEL_INFO(in,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
>>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL),
>>> + NULL
>>> +};
>>> +
>>> +static const struct hwmon_chip_info socfpga_chip_info = {
>>> + .ops = &socfpga_ops,
>>> + .info = socfpga_info,
>>> +};
>>> +
>>> +static int socfpga_add_channel(struct device *dev, const char *type,
>>> + u32 val, const char *label,
>>> + struct socfpga_hwmon_priv *priv)
>>> +{
>>> + int type_index;
>>> + struct socfpga_hwmon_chan *p;
>>> +
>>> + type_index = match_string(hwmon_types_str, ARRAY_SIZE(hwmon_types_str), type);
>>> + switch (type_index) {
>>> + case SOCFPGA_HWMON_TYPE_TEMP:
>>> + p = &priv->temperature;
>>> + break;
>>> + case SOCFPGA_HWMON_TYPE_VOLT:
>>> + p = &priv->voltage;
>>> + break;
>>> + default:
>>> + return -ENODATA;
>>> + }
>>> + if (p->n >= SOCFPGA_HWMON_MAXSENSORS)
>>> + return -ENOSPC;
>>> +
>>> + p->names[p->n] = label;
>>> + p->chan[p->n] = val;
>>> + p->n++;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int socfpga_probe_child_from_dt(struct device *dev,
>>> + struct device_node *child,
>>> + struct socfpga_hwmon_priv *priv)
>>> +{
>>> + struct device_node *grandchild;
>>> + const char *label;
>>> + const char *type;
>>> + u32 val;
>>> + int ret;
>>> +
>>> + if (of_property_read_string(child, "name", &type))
>>> + return dev_err_probe(dev, -EINVAL, "No type for %pOF\n", child);
>>> +
>>> + for_each_child_of_node(child, grandchild) {
>>> + ret = of_property_read_u32(grandchild, "reg", &val);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "missing reg property of %pOF\n",
>>> + grandchild);
>>> +
>>> + ret = of_property_read_string(grandchild, "label", &label);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "missing label propoerty of %pOF\n",
>>> + grandchild);
>>> + ret = socfpga_add_channel(dev, type, val, label, priv);
>>> + if (ret == -ENOSPC)
>>> + return dev_err_probe(dev, ret, "too many channels, only %d supported\n",
>>> + SOCFPGA_HWMON_MAXSENSORS);
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int socfpga_probe_from_dt(struct device *dev,
>>> + struct socfpga_hwmon_priv *priv)
>>> +{
>>> + const struct device_node *np = dev->of_node;
>>> + struct device_node *child;
>>> + int ret = 0;
>>> +
>>> + for_each_child_of_node(np, child) {
>>> + ret = socfpga_probe_child_from_dt(dev, child, priv);
>>> + if (ret)
>>> + break;
>>> + }
>>> + of_node_put(child);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int socfpga_hwmon_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct device *hwmon_dev;
>>> + struct socfpga_hwmon_priv *priv;
>>> + int ret;
>>> +
>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + priv->client.dev = dev;
>>> + priv->client.priv = priv;
>>> +
>>> + ret = socfpga_probe_from_dt(dev, priv);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Unable to probe from device tree\n");
>>> +
>>> + mutex_init(&priv->lock);
>>> + init_completion(&priv->completion);
>>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpgahwmon",
>>> + priv,
>>> + &socfpga_chip_info,
>>> + NULL);
>>> + if (IS_ERR(hwmon_dev))
>>> + return PTR_ERR(hwmon_dev);
>>> +
>>> + priv->chan = stratix10_svc_request_channel_byname(&priv->client,
>>> + SVC_CLIENT_HWMON);
>>
>> This is racy: hwmon attributes exist here, and priv->chan may be accessed before
>> it is set.
>
> Fixed in v2.
>
>>
>>> + if (IS_ERR(priv->chan))
>>> + return dev_err_probe(dev, PTR_ERR(priv->chan),
>>> + "couldn't get service channel %s\n",
>>> + SVC_CLIENT_RSU);
>>> +
>>> + platform_set_drvdata(pdev, priv);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int socfpga_hwmon_remove(struct platform_device *pdev)
>>> +{
>>> + struct socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
>>> +
>>> + stratix10_svc_free_channel(priv->chan);
>>
>> This releases the channel before the hwmon device is released.
>> Another race condition.
>
> fixed in V2.
>
> Thanks for the review!
>
> Dinh
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-17 21:51 ` Guenter Roeck
@ 2023-04-18 17:29 ` Dinh Nguyen
2023-04-19 11:46 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Dinh Nguyen @ 2023-04-18 17:29 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: dinguyen, devicetree, robh+dt, krzysztof.kozlowski+dt,
linux-kernel, jdelvare, Andy Shevchenko
On 4/17/2023 4:51 PM, Guenter Roeck wrote:
> On 4/17/23 13:55, Dinh Nguyen wrote:
>>
>> On 4/10/2023 9:44 PM, Guenter Roeck wrote:
>>> On 4/10/23 08:33, dinh.nguyen@linux.intel.com wrote:
>>>> From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>>>>
>>>> The driver supports 64-bit SoCFPGA platforms for temperature and
>>>> voltage
>>>> reading using the platform's SDM(Secure Device Manager). The driver
>>>> also uses the Stratix10 Service layer driver.
>>>>
>>>> This driver only supports OF SoCFPGA 64-bit platforms.
>>>>
>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>> Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
>>>> ---
>>>> Documentation/hwmon/index.rst | 1 +
>>>> Documentation/hwmon/socfpga-hwmon.rst | 30 ++
>>>> drivers/firmware/stratix10-svc.c | 18 +-
>>>
>>> Changes outside the hwmon subsystem need to be in a separate patch.
>>
>> will separate...
>>
>>>
>>>> drivers/hwmon/Kconfig | 11 +
>>>> drivers/hwmon/Makefile | 1 +
>>>> drivers/hwmon/socfpga-hwmon.c | 406
>>>> ++++++++++++++++++
>>>> include/linux/firmware/intel/stratix10-smc.h | 34 ++
>>>> .../firmware/intel/stratix10-svc-client.h | 6 +
>>>> 8 files changed, 506 insertions(+), 1 deletion(-)
>>>> create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
>>>> create mode 100644 drivers/hwmon/socfpga-hwmon.c
>>>>
>>>>
>> ...
>>>> +
>>>> +enum hwmon_type_op {
>>>> + SOCFPGA_HWMON_TYPE_TEMP,
>>>> + SOCFPGA_HWMON_TYPE_VOLT,
>>>> + SOCFPGA_HWMON_TYPE_MAX
>>>
>>> Unused define
>>
>> Removed.
>>
>>>
>>>> +};
>>>> +
>>>> +static const char *const hwmon_types_str[] = { "temperature",
>>>> "voltage" };
>>>> +
>>>> +static umode_t socfpga_is_visible(const void *dev,
>>>> + enum hwmon_sensor_types type,
>>>> + u32 attr, int chan)
>>>> +{
>>>> + switch (type) {
>>>> + case hwmon_temp:
>>>> + case hwmon_in:
>>>> + return 0444;
>>>> + default:
>>>> + return 0;
>>>> + }
>>>> +}
>>>> +
>>>> +static void socfpga_smc_callback(struct stratix10_svc_client *client,
>>>> + struct stratix10_svc_cb_data *data)
>>>> +{
>>>> + struct socfpga_hwmon_priv *priv = client->priv;
>>>> + struct arm_smccc_res *res = data->kaddr1;
>>>> +
>>>> + if (data->status == BIT(SVC_STATUS_OK)) {
>>>> + if (priv->msg.command == COMMAND_HWMON_READTEMP)
>>>> + priv->temperature.value = res->a0;
>>>> + else
>>>> + priv->voltage.value = res->a0;
>>>> + } else
>>>> + dev_err(client->dev, "%s returned 0x%lX\n", __func__,
>>>> res->a0);
>>>> +
>>>
>>> Missing { } in else branch. Please run checkpatch --strict and fix
>>> continuation line alignment issues as well as unbalanced if/else
>>> reports.
>> Will do.
>>>
>>>> + complete(&priv->completion);
>>>> +}
>>>> +
>>>> +static int socfpga_hwmon_send(struct socfpga_hwmon_priv *priv)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + priv->client.receive_cb = socfpga_smc_callback;
>>>> +
>>>> + ret = stratix10_svc_send(priv->chan, &priv->msg);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + if (!wait_for_completion_timeout(&priv->completion,
>>>> HWMON_TIMEOUT)) {
>>>> + dev_err(priv->client.dev, "SMC call timeout!\n");
>>>> + return -ETIMEDOUT;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv
>>>> *priv)
>>>> +{
>>>> + int value = priv->temperature.value;
>>>> +
>>>> + if (!(value & ETEMP_ERROR))
>>>> + return 0;
>>>> +
>>>
>>> This is odd. int is normally 32 bit, this function is called from
>>> socfpga_read() for temperatures, which presumably are defined
>>> as "signed 32-bit fixed point binary". That means that negative
>>> temperatures would be treated as errors. Please verify.
>>
>> That's correct, if bit 31 is set, then it indicates an error.
>>
>
> This ...
>
>>>
>>>> + dev_err(priv->client.dev, "temperature sensor code 0x%08x\n",
>>>> value);
>>>> +
>>>
>>> Please don't clog the log with such messages.
>>
>> Removed.
>>
>>>
>>>> + value &= ~ETEMP_ERROR;
>>>> + switch (value) {
>>>> + case ETEMP_NOT_PRESENT:
>>>> + return -ENOENT;
>>>> + case ETEMP_CORRUPT:
>>>> + case ETEMP_NOT_INITIALIZED:
>>>> + return -ENODATA;
>>>> + case ETEMP_BUSY:
>>>> + return -EBUSY;
>>>> + case ETEMP_INACTIVE:
>>>> + case ETEMP_TIMEOUT:
>>>> + case ETEMP_TOO_OLD:
>>>> + return -EAGAIN;
>>>> + default:
>>>> + /* Unknown error */
>>>> + return -EINVAL;
>>>
>>> Should be -EIO.
>>>
>> Replaced.
>>>> + }
>>>> +}
>>>> +
>>>> +static int socfpga_read(struct device *dev, enum
>>>> hwmon_sensor_types type,
>>>> + u32 attr, int chan, long *val)
>>>> +{
>>>> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
>>>> + int ret;
>>>> +
>>>> + mutex_lock(&priv->lock);
>>>> + reinit_completion(&priv->completion);
>>>> +
>>>> + switch (type) {
>>>> + case hwmon_temp:
>>>> + priv->msg.arg[0] = BIT_ULL(priv->temperature.chan[chan]);
>>>> + priv->msg.command = COMMAND_HWMON_READTEMP;
>>>> + if (socfpga_hwmon_send(priv))
>>>> + goto status_done;
>>>> +
>>>> + ret = socfpga_hwmon_err_to_errno(priv);
>>>> + if (ret)
>>>> + break;
>>>> + /*
>>>> + * The Temperature Sensor IP core returns the Celsius
>>>> + * temperature value in signed 32-bit fixed point binary
>
> ... and this contradict each other. If bit 31 indicates an error,
> this can not be a signed 32-bit value.
>
You're right! I've re-read the spec and should have the the code look
for the specific error values:
0x80000000 - inactive
0x80000001 - old value
0x80000002 - invalid channel
0x80000003 - corrupted.
...
Dinh
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-18 17:29 ` Dinh Nguyen
@ 2023-04-19 11:46 ` Andy Shevchenko
2023-04-20 14:46 ` Dinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2023-04-19 11:46 UTC (permalink / raw)
To: Dinh Nguyen
Cc: Guenter Roeck, linux-hwmon, dinguyen, devicetree, robh+dt,
krzysztof.kozlowski+dt, linux-kernel, jdelvare
On Tue, Apr 18, 2023 at 12:29:40PM -0500, Dinh Nguyen wrote:
> On 4/17/2023 4:51 PM, Guenter Roeck wrote:
> > On 4/17/23 13:55, Dinh Nguyen wrote:
...
> > ... and this contradict each other. If bit 31 indicates an error,
> > this can not be a signed 32-bit value.
> >
> You're right! I've re-read the spec and should have the the code look for
> the specific error values:
>
> 0x80000000 - inactive
> 0x80000001 - old value
> 0x80000002 - invalid channel
> 0x80000003 - corrupted.
No, they are not hex. Probably you need to define an error space with it, but
at least just use signed _decimal_ values.
Instead of BIT(31) this should go as
#define ..._ERR_BASE INT_MIN // or equivalent if the type is not int
#define ..._ERR_MAX ... // or whatever name is better
Then in your code
if (value >= _ERR_MAX)
return 0;
err = _ERR_MAX - value;
switch (err) {
...
}
P.S. I asked during internal review if the values are bit fielded when errors.
AFAIU that time they are, now it seems different.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-19 11:46 ` Andy Shevchenko
@ 2023-04-20 14:46 ` Dinh Nguyen
2023-04-21 9:36 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Dinh Nguyen @ 2023-04-20 14:46 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Guenter Roeck, linux-hwmon, dinguyen, devicetree, robh+dt,
krzysztof.kozlowski+dt, linux-kernel, jdelvare
On 4/19/2023 6:46 AM, Andy Shevchenko wrote:
> On Tue, Apr 18, 2023 at 12:29:40PM -0500, Dinh Nguyen wrote:
>> On 4/17/2023 4:51 PM, Guenter Roeck wrote:
>>> On 4/17/23 13:55, Dinh Nguyen wrote:
> ...
>
>>> ... and this contradict each other. If bit 31 indicates an error,
>>> this can not be a signed 32-bit value.
>>>
>> You're right! I've re-read the spec and should have the the code look for
>> the specific error values:
>>
>> 0x80000000 - inactive
>> 0x80000001 - old value
>> 0x80000002 - invalid channel
>> 0x80000003 - corrupted.
> No, they are not hex. Probably you need to define an error space with it, but
> at least just use signed _decimal_ values.
>
> Instead of BIT(31) this should go as
>
> #define ..._ERR_BASE INT_MIN // or equivalent if the type is not int
> #define ..._ERR_MAX ... // or whatever name is better
>
> Then in your code
>
> if (value >= _ERR_MAX)
> return 0;
>
> err = _ERR_MAX - value;
> switch (err) {
> ...
> }
>
> P.S. I asked during internal review if the values are bit fielded when errors.
> AFAIU that time they are, now it seems different.
Can I ask what's wrong with this simple implementation?
static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
{
int value = priv->temperature.value;
switch (value) {
case ETEMP_NOT_PRESENT:
return -ENOENT;
case ETEMP_CORRUPT:
case ETEMP_NOT_INITIALIZED:
return -ENODATA;
case ETEMP_BUSY:
return -EBUSY;
case ETEMP_INACTIVE:
case ETEMP_TIMEOUT:
case ETEMP_TOO_OLD:
return -EAGAIN;
default:
/* No error */
return 0;
}
}
Dinh
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms
2023-04-20 14:46 ` Dinh Nguyen
@ 2023-04-21 9:36 ` Andy Shevchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2023-04-21 9:36 UTC (permalink / raw)
To: Dinh Nguyen
Cc: Guenter Roeck, linux-hwmon, dinguyen, devicetree, robh+dt,
krzysztof.kozlowski+dt, linux-kernel, jdelvare
On Thu, Apr 20, 2023 at 09:46:20AM -0500, Dinh Nguyen wrote:
> On 4/19/2023 6:46 AM, Andy Shevchenko wrote:
> > On Tue, Apr 18, 2023 at 12:29:40PM -0500, Dinh Nguyen wrote:
> > > On 4/17/2023 4:51 PM, Guenter Roeck wrote:
> > > > On 4/17/23 13:55, Dinh Nguyen wrote:
...
> > > > ... and this contradict each other. If bit 31 indicates an error,
> > > > this can not be a signed 32-bit value.
> > > >
> > > You're right! I've re-read the spec and should have the the code look for
> > > the specific error values:
> > >
> > > 0x80000000 - inactive
> > > 0x80000001 - old value
> > > 0x80000002 - invalid channel
> > > 0x80000003 - corrupted.
> > No, they are not hex. Probably you need to define an error space with it, but
> > at least just use signed _decimal_ values.
> >
> > Instead of BIT(31) this should go as
> >
> > #define ..._ERR_BASE INT_MIN // or equivalent if the type is not int
> > #define ..._ERR_MAX ... // or whatever name is better
> >
> > Then in your code
> >
> > if (value >= _ERR_MAX)
> > return 0;
> >
> > err = _ERR_MAX - value;
> > switch (err) {
> > ...
> > }
> >
> > P.S. I asked during internal review if the values are bit fielded when errors.
> > AFAIU that time they are, now it seems different.
>
> Can I ask what's wrong with this simple implementation?
Technically, nothing, but from understanding point of view it would be better
to have explicit ranges of error number space vs. actual value space.
The idea in the firmware of that device seems to me similar to what we have in
the Linux kernel. Note, it may be not _so_ explicitly, but the error number
space is limited by a PAGE_SIZE. All the same may be applied here.
> static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
> {
> int value = priv->temperature.value;
>
> switch (value) {
> case ETEMP_NOT_PRESENT:
> return -ENOENT;
> case ETEMP_CORRUPT:
> case ETEMP_NOT_INITIALIZED:
> return -ENODATA;
> case ETEMP_BUSY:
> return -EBUSY;
> case ETEMP_INACTIVE:
> case ETEMP_TIMEOUT:
> case ETEMP_TOO_OLD:
> return -EAGAIN;
> default:
> /* No error */
> return 0;
> }
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-04-21 9:36 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-10 15:33 [PATCH 1/5] units: add a macro for MILLIVOLT_PER_VOLT dinh.nguyen
2023-04-10 15:33 ` [PATCH 2/5] dt-bindings: hwmon: intel: add hardware monitor bindings for SoCFPGA dinh.nguyen
2023-04-10 23:48 ` Rob Herring
2023-04-11 6:53 ` Krzysztof Kozlowski
2023-04-10 15:33 ` [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms dinh.nguyen
2023-04-11 2:44 ` Guenter Roeck
2023-04-17 20:55 ` Dinh Nguyen
2023-04-17 21:51 ` Guenter Roeck
2023-04-18 17:29 ` Dinh Nguyen
2023-04-19 11:46 ` Andy Shevchenko
2023-04-20 14:46 ` Dinh Nguyen
2023-04-21 9:36 ` Andy Shevchenko
2023-04-10 15:33 ` [PATCH 4/5] MAINTAINERS: add Dinh Nguyen for socfpga-hwmon driver dinh.nguyen
2023-04-10 15:33 ` [PATCH 5/5] arm64: dts: socfpga: add hwmon properties dinh.nguyen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).