| Message ID | 20230410153314.27127-3-dinh.nguyen@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/5] units: add a macro for MILLIVOLT_PER_VOLT | expand |
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, > };
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
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 >
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
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.
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
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; > } > }
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, };