* [PATCH v2 0/3] hwmon: xgene: Add support for X-Gene hwmon driver
@ 2016-07-12 0:30 Hoan Tran
2016-07-12 0:30 ` [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation Hoan Tran
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Hoan Tran @ 2016-07-12 0:30 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, Rob Herring
Cc: Jassi Brar, Ashwin Chaugule, linux-hwmon, linux-doc,
linux-kernel, linux-arm-kernel, devicetree, Duc Dang, lho,
Hoan Tran
This patch set adds hardware temperature and power reading support for
APM X-Gene SoC using the mailbox communication interface.
For device tree, it is the standard DT mailbox.
For ACPI, it is the PCC mailbox.
For ACPI, tested with this patch[1] which supports PCC subspace 2
[1] http://www.spinics.net/lists/linux-acpi/msg66041.html
- [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
v2
- Increase power reading accurateness by using 2 registers
(a register for Watt, another for milli-Watt)
- Remove power reading for SoC
- Fix review comments from Guenter
v1
- Initial
Hoan Tran (3):
Documentation: dtb: xgene: Add hwmon dts binding documentation
hwmon: xgene: Add hwmon driver
arm64: dts: apm: Add X-Gene SoC hwmon to device tree
.../devicetree/bindings/hwmon/apm-xgene-hwmon.txt | 14 +
Documentation/hwmon/xgene-hwmon | 30 +
arch/arm64/boot/dts/apm/apm-shadowcat.dtsi | 5 +
arch/arm64/boot/dts/apm/apm-storm.dtsi | 5 +
drivers/hwmon/Kconfig | 7 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/xgene-hwmon.c | 772 +++++++++++++++++++++
7 files changed, 834 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt
create mode 100644 Documentation/hwmon/xgene-hwmon
create mode 100644 drivers/hwmon/xgene-hwmon.c
--
1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation 2016-07-12 0:30 [PATCH v2 0/3] hwmon: xgene: Add support for X-Gene hwmon driver Hoan Tran @ 2016-07-12 0:30 ` Hoan Tran 2016-07-16 19:28 ` Rob Herring 2016-07-12 0:30 ` [PATCH v2 2/3] hwmon: xgene: Add hwmon driver Hoan Tran 2016-07-12 0:30 ` [PATCH v2 3/3] arm64: dts: apm: Add X-Gene SoC hwmon to device tree Hoan Tran 2 siblings, 1 reply; 7+ messages in thread From: Hoan Tran @ 2016-07-12 0:30 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, Jonathan Corbet, Rob Herring Cc: Jassi Brar, Ashwin Chaugule, linux-hwmon, linux-doc, linux-kernel, linux-arm-kernel, devicetree, Duc Dang, lho, Hoan Tran This patch adds the APM X-Gene hwmon device tree node documentation. Signed-off-by: Hoan Tran <hotran@apm.com> --- .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt diff --git a/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt new file mode 100644 index 0000000..59b3855 --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt @@ -0,0 +1,14 @@ +APM X-Gene hwmon driver + +APM X-Gene SOC sensors are accessed over the "SLIMpro" mailbox. + +Required properties : + - compatible : should be "apm,xgene-slimpro-hwmon" + - mboxes : use the label reference for the mailbox as the first parameter. + The second parameter is the channel number. + +Example : + hwmonslimpro { + compatible = "apm,xgene-slimpro-hwmon"; + mboxes = <&mailbox 7>; + }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation 2016-07-12 0:30 ` [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation Hoan Tran @ 2016-07-16 19:28 ` Rob Herring 0 siblings, 0 replies; 7+ messages in thread From: Rob Herring @ 2016-07-16 19:28 UTC (permalink / raw) To: Hoan Tran Cc: Jean Delvare, Guenter Roeck, Jonathan Corbet, Jassi Brar, Ashwin Chaugule, linux-hwmon, linux-doc, linux-kernel, linux-arm-kernel, devicetree, Duc Dang, lho On Mon, Jul 11, 2016 at 05:30:14PM -0700, Hoan Tran wrote: > This patch adds the APM X-Gene hwmon device tree node documentation. > > Signed-off-by: Hoan Tran <hotran@apm.com> > --- > .../devicetree/bindings/hwmon/apm-xgene-hwmon.txt | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/apm-xgene-hwmon.txt Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] hwmon: xgene: Add hwmon driver 2016-07-12 0:30 [PATCH v2 0/3] hwmon: xgene: Add support for X-Gene hwmon driver Hoan Tran 2016-07-12 0:30 ` [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation Hoan Tran @ 2016-07-12 0:30 ` Hoan Tran 2016-07-16 16:35 ` Guenter Roeck 2016-07-12 0:30 ` [PATCH v2 3/3] arm64: dts: apm: Add X-Gene SoC hwmon to device tree Hoan Tran 2 siblings, 1 reply; 7+ messages in thread From: Hoan Tran @ 2016-07-12 0:30 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, Jonathan Corbet, Rob Herring Cc: Jassi Brar, Ashwin Chaugule, linux-hwmon, linux-doc, linux-kernel, linux-arm-kernel, devicetree, Duc Dang, lho, Hoan Tran This patch adds hardware temperature and power reading support for APM X-Gene SoC using the mailbox communication interface. Signed-off-by: Hoan Tran <hotran@apm.com> --- Documentation/hwmon/xgene-hwmon | 30 ++ drivers/hwmon/Kconfig | 7 + drivers/hwmon/Makefile | 1 + drivers/hwmon/xgene-hwmon.c | 772 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 810 insertions(+) create mode 100644 Documentation/hwmon/xgene-hwmon create mode 100644 drivers/hwmon/xgene-hwmon.c diff --git a/Documentation/hwmon/xgene-hwmon b/Documentation/hwmon/xgene-hwmon new file mode 100644 index 0000000..6ec50ed --- /dev/null +++ b/Documentation/hwmon/xgene-hwmon @@ -0,0 +1,30 @@ +Kernel driver xgene-hwmon +======================== + +Supported chips: + * APM X-Gene SoC + +Description +----------- + +This driver adds hardware temperature and power reading support for +APM X-Gene SoC using the mailbox communication interface. +For device tree, it is the standard DT mailbox. +For ACPI, it is the PCC mailbox. + +The following sensors are supported + + * Temperature + - SoC on-die temperature in milli-degree C + - Alarm when high/over temperature occurs + * Power + - CPU power in uW + - IO power in uW + +sysfs-Interface +--------------- + +temp0_input - SoC on-die temperature (milli-degree C) +temp0_critical_alarm - An 1 would indicates on-die temperature exceeded threshold +power0_input - CPU power in (uW) +power1_input - IO power in (uW) diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index ff94007..55218c6 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1787,6 +1787,13 @@ config SENSORS_ULTRA45 This driver provides support for the Ultra45 workstation environmental sensors. +config SENSORS_XGENE + tristate "APM X-Gene SoC hardware monitoring driver" + depends on XGENE_SLIMPRO_MBOX || PCC + help + If you say yes here you get support for the temperature + and power sensors for APM X-Gene SoC. + if ACPI comment "ACPI drivers" diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 2ef5b7c..a2460dd 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -162,6 +162,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o +obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o obj-$(CONFIG_PMBUS) += pmbus/ diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c new file mode 100644 index 0000000..03917e3 --- /dev/null +++ b/drivers/hwmon/xgene-hwmon.c @@ -0,0 +1,772 @@ +/* + * APM X-Gene SoC Hardware Monitoring Driver + * + * Copyright (c) 2016, Applied Micro Circuits Corporation + * Author: Loc Ho <lho@apm.com> + * Hoan Tran <hotran@apm.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + * + * This driver provides the following features: + * - Retrieve CPU total power (uW) + * - Retrieve IO total power (uW) + * - Retrieve SoC temperature (milli-degree C) and alarm + */ +#include <linux/acpi.h> +#include <linux/dma-mapping.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/kfifo.h> +#include <linux/interrupt.h> +#include <linux/mailbox_controller.h> +#include <linux/mailbox_client.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <acpi/acpi_io.h> +#include <acpi/pcc.h> + +/* SLIMpro message defines */ +#define MSG_TYPE_DBG 0 +#define MSG_TYPE_ERR 7 +#define MSG_TYPE_PWRMGMT 9 + +#define MSG_TYPE(v) (((v) & 0xF0000000) >> 28) +#define MSG_TYPE_SET(v) (((v) << 28) & 0xF0000000) +#define MSG_SUBTYPE(v) (((v) & 0x0F000000) >> 24) +#define MSG_SUBTYPE_SET(v) (((v) << 24) & 0x0F000000) + +#define DBG_SUBTYPE_SENSOR_READ 4 +#define SENSOR_RD_MSG 0x04FFE902 +#define SENSOR_RD_EN_ADDR(a) ((a) & 0x000FFFFF) +#define PMD_PWR_REG 0x20 +#define PMD_PWR_MW_REG 0x26 +#define SOC_PWR_REG 0x21 +#define SOC_PWR_MW_REG 0x27 +#define SOC_TEMP_REG 0x10 + +#define TEMP_NEGATIVE_BIT 8 + +#define PWRMGMT_SUBTYPE_TPC 1 +#define TPC_ALARM 2 +#define TPC_GET_ALARM 3 +#define TPC_CMD(v) (((v) & 0x00FF0000) >> 16) +#define TPC_CMD_SET(v) (((v) << 16) & 0x00FF0000) +#define TPC_EN_MSG(hndl, cmd, type) \ + (MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \ + MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type) + +/* PCC defines */ +#define PCC_SIGNATURE_MASK 0x50424300 +#define PCCC_GENERATE_DB_INT BIT(15) +#define PCCS_CMD_COMPLETE BIT(0) +#define PCCS_SCI_DOORBEL BIT(1) +#define PCCS_PLATFORM_NOTIFICATION BIT(3) +/* + * Arbitrary retries in case the remote processor is slow to respond + * to PCC commands + */ +#define PCC_NUM_RETRIES 500 + +#define ASYNC_MSG_FIFO_SIZE 16 +#define MBOX_OP_TIMEOUTMS 1000 + +#define WATT_TO_mWATT(x) ((x) * 1000) +#define mWATT_TO_uWATT(x) ((x) * 1000) +#define CELSIUS_TO_mCELSIUS(x) ((x) * 1000) + +#define to_xgene_hwmon_dev(cl) \ + container_of(cl, struct xgene_hwmon_dev, mbox_client) + +struct slimpro_resp_msg { + u32 msg; + u32 param1; + u32 param2; +} __packed; + +struct xgene_hwmon_dev { + struct device *dev; + struct mbox_chan *mbox_chan; + struct mbox_client mbox_client; + int mbox_idx; + + spinlock_t kfifo_lock; + struct mutex rd_mutex; + struct completion rd_complete; + int resp_pending; + struct slimpro_resp_msg sync_msg; + + struct work_struct workq; + struct kfifo_rec_ptr_1 async_msg_fifo; + + struct device *hwmon_dev; + bool temp_critical_alarm; + + phys_addr_t comm_base_addr; + void *pcc_comm_addr; + u64 usecs_lat; +}; + +/* + * This function tests and clears a bitmask then returns its old value + */ +static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask) +{ + u16 ret, val; + + val = readw_relaxed(addr); + ret = val & mask; + val &= ~mask; + writew_relaxed(val, addr); + + return ret; +} + +static int xgene_hwmon_decode_temp(u32 val) +{ + unsigned long temp = val; + + /* Convert 9 bit signed temperature to integer */ + if (test_and_clear_bit(TEMP_NEGATIVE_BIT, &temp)) + return (temp - 256); + else + return temp; +} + +static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg) +{ + struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; + void *ptr = generic_comm_base + 1; + int rc, i; + u16 val; + + mutex_lock(&ctx->rd_mutex); + init_completion(&ctx->rd_complete); + ctx->resp_pending = true; + + /* Write signature for subspace */ + writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx, + &generic_comm_base->signature); + + /* Write to the shared command region */ + writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT, + &generic_comm_base->command); + + /* Flip CMD COMPLETE bit */ + val = readw_relaxed(&generic_comm_base->status); + val &= ~PCCS_CMD_COMPLETE; + writew_relaxed(val, &generic_comm_base->status); + + /* Copy the message to the PCC comm space */ + for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++) + writel_relaxed(msg[i], ptr + i * 4); + + /* Ring the doorbell */ + rc = mbox_send_message(ctx->mbox_chan, msg); + if (rc < 0) { + dev_err(ctx->dev, "Mailbox send error %d\n", rc); + goto err; + } + if (!wait_for_completion_timeout(&ctx->rd_complete, + usecs_to_jiffies(ctx->usecs_lat))) { + dev_err(ctx->dev, "Mailbox operation timed out\n"); + rc = -ETIMEDOUT; + goto err; + } + + /* Check for invalid data */ + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { + rc = -EINVAL; + goto err; + } + + msg[0] = ctx->sync_msg.msg; + msg[1] = ctx->sync_msg.param1; + msg[2] = ctx->sync_msg.param2; + +err: + mbox_chan_txdone(ctx->mbox_chan, 0); + ctx->resp_pending = false; + mutex_unlock(&ctx->rd_mutex); + return rc; +} + +static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg) +{ + int rc; + + mutex_lock(&ctx->rd_mutex); + init_completion(&ctx->rd_complete); + ctx->resp_pending = true; + + rc = mbox_send_message(ctx->mbox_chan, msg); + if (rc < 0) { + dev_err(ctx->dev, "Mailbox send error %d\n", rc); + goto err; + } + + if (!wait_for_completion_timeout(&ctx->rd_complete, + msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) { + dev_err(ctx->dev, "Mailbox operation timed out\n"); + rc = -ETIMEDOUT; + goto err; + } + + /* Check for invalid data */ + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { + rc = -EINVAL; + goto err; + } + + msg[0] = ctx->sync_msg.msg; + msg[1] = ctx->sync_msg.param1; + msg[2] = ctx->sync_msg.param2; + +err: + ctx->resp_pending = false; + mutex_unlock(&ctx->rd_mutex); + return rc; +} + +static int xgene_hwmon_reg_map_rd(struct xgene_hwmon_dev *ctx, u32 addr, + u32 *data) +{ + u32 msg[3]; + int rc; + + msg[0] = SENSOR_RD_MSG; + msg[1] = SENSOR_RD_EN_ADDR(addr); + msg[2] = 0; + + if (acpi_disabled) + rc = xgene_hwmon_rd(ctx, msg); + else + rc = xgene_hwmon_pcc_rd(ctx, msg); + + if (rc < 0) + return rc; + + *data = msg[1]; + + return rc; +} + +static int xgene_hwmon_get_notification_msg(struct xgene_hwmon_dev *ctx, + u32 *amsg) +{ + u32 msg[3]; + int rc; + + msg[0] = TPC_EN_MSG(PWRMGMT_SUBTYPE_TPC, TPC_GET_ALARM, 0); + msg[1] = 0; + msg[2] = 0; + + rc = xgene_hwmon_pcc_rd(ctx, msg); + if (rc < 0) + return rc; + + amsg[0] = msg[0]; + amsg[1] = msg[1]; + amsg[2] = msg[2]; + + return rc; +} + +static int xgene_hwmon_get_cpu_pwr(struct xgene_hwmon_dev *ctx, u32 *val) +{ + u32 watt, mwatt; + int rc; + + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_REG, &watt); + if (rc < 0) + return rc; + + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, &mwatt); + if (rc < 0) + return rc; + + *val = WATT_TO_mWATT(watt) + mwatt; + return 0; +} + +static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val) +{ + u32 watt, mwatt; + int rc; + + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, &watt); + if (rc < 0) + return rc; + + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_MW_REG, &mwatt); + if (rc < 0) + return rc; + + *val = WATT_TO_mWATT(watt) + mwatt; + return 0; +} + +static int xgene_hwmon_get_temp(struct xgene_hwmon_dev *ctx, u32 *val) +{ + return xgene_hwmon_reg_map_rd(ctx, SOC_TEMP_REG, val); +} + +/* + * Sensor temperature/power functions + */ +static ssize_t xgene_hwmon_show_temp(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); + int rc, temp; + u32 val; + + rc = xgene_hwmon_get_temp(ctx, &val); + if (rc < 0) + return rc; + + temp = xgene_hwmon_decode_temp(val); + + return snprintf(buf, PAGE_SIZE, "%d\n", CELSIUS_TO_mCELSIUS(temp)); +} + +static ssize_t xgene_hwmon_show_temp_label(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return snprintf(buf, PAGE_SIZE, "SoC Temperature\n"); +} + +static ssize_t +xgene_hwmon_show_temp_critical_alarm(struct device *dev, + struct device_attribute *devattr, + char *buf) +{ + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); + + return snprintf(buf, PAGE_SIZE, "%d\n", ctx->temp_critical_alarm); +} + + +static ssize_t xgene_hwmon_show_cpu_pwr_label(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return snprintf(buf, PAGE_SIZE, "CPU power\n"); +} + +static ssize_t xgene_hwmon_show_io_pwr_label(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return snprintf(buf, PAGE_SIZE, "IO power\n"); +} + +static ssize_t xgene_hwmon_show_cpu_pwr(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); + u32 val; + int rc; + + rc = xgene_hwmon_get_cpu_pwr(ctx, &val); + if (rc < 0) + return rc; + + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); +} + +static ssize_t xgene_hwmon_show_io_pwr(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); + u32 val; + int rc; + + rc = xgene_hwmon_get_io_pwr(ctx, &val); + if (rc < 0) + return rc; + + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); +} + +static SENSOR_DEVICE_ATTR(temp0_label, S_IRUGO, xgene_hwmon_show_temp_label, + NULL, 0); +static SENSOR_DEVICE_ATTR(temp0_input, S_IRUGO, xgene_hwmon_show_temp, + NULL, 0); +static SENSOR_DEVICE_ATTR(temp0_critical_alarm, S_IRUGO, + xgene_hwmon_show_temp_critical_alarm, NULL, 0); + +static SENSOR_DEVICE_ATTR(power0_label, S_IRUGO, + xgene_hwmon_show_cpu_pwr_label, NULL, 0); +static SENSOR_DEVICE_ATTR(power0_input, S_IRUGO, xgene_hwmon_show_cpu_pwr, + NULL, 0); + +static SENSOR_DEVICE_ATTR(power1_label, S_IRUGO, + xgene_hwmon_show_io_pwr_label, NULL, 1); +static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, xgene_hwmon_show_io_pwr, + NULL, 1); + +static struct attribute *xgene_hwmon_attrs[] = { + &sensor_dev_attr_temp0_label.dev_attr.attr, + &sensor_dev_attr_temp0_input.dev_attr.attr, + &sensor_dev_attr_temp0_critical_alarm.dev_attr.attr, + &sensor_dev_attr_power0_label.dev_attr.attr, + &sensor_dev_attr_power0_input.dev_attr.attr, + &sensor_dev_attr_power1_label.dev_attr.attr, + &sensor_dev_attr_power1_input.dev_attr.attr, + NULL, +}; +ATTRIBUTE_GROUPS(xgene_hwmon); + +static int xgene_hwmon_tpc_alarm(struct xgene_hwmon_dev *ctx, + struct slimpro_resp_msg *amsg) +{ + char name[40]; + + ctx->temp_critical_alarm = amsg->param2 ? true : false; + snprintf(name, sizeof(name), "temp0_critical_alarm"); + sysfs_notify(&ctx->dev->kobj, NULL, name); + + return 0; +} + +static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx, + struct slimpro_resp_msg *amsg) +{ + if ((MSG_SUBTYPE(amsg->msg) == PWRMGMT_SUBTYPE_TPC) && + (TPC_CMD(amsg->msg) == TPC_ALARM)) + xgene_hwmon_tpc_alarm(ctx, amsg); +} + +/* + * This function is called to process async work queue + */ +static void xgene_hwmon_evt_work(struct work_struct *work) +{ + struct slimpro_resp_msg amsg; + struct xgene_hwmon_dev *ctx; + int ret; + + ctx = container_of(work, struct xgene_hwmon_dev, workq); + while (kfifo_out_spinlocked(&ctx->async_msg_fifo, &amsg, + sizeof(struct slimpro_resp_msg), + &ctx->kfifo_lock)) { + /* + * If PCC, send a consumer command to Platform to get info + * If Slimpro Mailbox, get message from specific FIFO + */ + if (!acpi_disabled) { + ret = xgene_hwmon_get_notification_msg(ctx, + (u32 *)&amsg); + if (ret < 0) + continue; + } + + if (MSG_TYPE(amsg.msg) == MSG_TYPE_PWRMGMT) + xgene_hwmon_process_pwrmsg(ctx, &amsg); + } +} + +/* + * This function is called when the SLIMpro Mailbox received a message + */ +static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg) +{ + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); + struct slimpro_resp_msg amsg; + + /* + * Response message format: + * msg[0] is the return code of the operation + * msg[1] is the first parameter word + * msg[2] is the second parameter word + * + * As message only supports dword size, just assign it. + */ + + /* Check for sync query */ + if (ctx->resp_pending && + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { + ctx->sync_msg.msg = ((u32 *) msg)[0]; + ctx->sync_msg.param1 = ((u32 *) msg)[1]; + ctx->sync_msg.param2 = ((u32 *) msg)[2]; + + /* Operation waiting for response */ + complete(&ctx->rd_complete); + + return; + } + + amsg.msg = ((u32 *) msg)[0]; + amsg.param1 = ((u32 *) msg)[1]; + amsg.param2 = ((u32 *) msg)[2]; + + /* Enqueue to the FIFO */ + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, + sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock); + /* Schedule the bottom handler */ + schedule_work(&ctx->workq); +} + +/* + * This function is called when the PCC Mailbox received a message + */ +static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg) +{ + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); + struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; + struct slimpro_resp_msg amsg; + + msg = generic_comm_base + 1; + /* Check if platform sends interrupt */ + if (!xgene_word_tst_and_clr(&generic_comm_base->status, + PCCS_SCI_DOORBEL)) + return; + + /* + * Response message format: + * msg[0] is the return code of the operation + * msg[1] is the first parameter word + * msg[2] is the second parameter word + * + * As message only supports dword size, just assign it. + */ + + /* Check for sync query */ + if (ctx->resp_pending && + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { + + /* Check if platform completes command */ + if (xgene_word_tst_and_clr(&generic_comm_base->status, + PCCS_CMD_COMPLETE)) { + ctx->sync_msg.msg = ((u32 *) msg)[0]; + ctx->sync_msg.param1 = ((u32 *) msg)[1]; + ctx->sync_msg.param2 = ((u32 *) msg)[2]; + + /* Operation waiting for response */ + complete(&ctx->rd_complete); + + return; + } + } + + /* + * Platform notifies interrupt to OSPM. + * OPSM schedules a consumer command to get this information + * in a workqueue. Platform must wait until OSPM has issued + * a consumer command that serves this notification. + */ + + /* Enqueue to the FIFO */ + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, + sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock); + /* Schedule the bottom handler */ + schedule_work(&ctx->workq); +} + +static void xgene_hwmon_tx_done(struct mbox_client *cl, void *msg, int ret) +{ + if (ret) { + dev_dbg(cl->dev, "TX did not complete: CMD sent:%x, ret:%d\n", + *(u16 *) msg, ret); + } else { + dev_dbg(cl->dev, "TX completed. CMD sent:%x, ret:%d\n", + *(u16 *) msg, ret); + } +} + +static int xgene_hwmon_probe(struct platform_device *pdev) +{ + struct xgene_hwmon_dev *ctx; + struct mbox_client *cl; + int rc; + + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + ctx->dev = &pdev->dev; + platform_set_drvdata(pdev, ctx); + cl = &ctx->mbox_client; + + /* Request mailbox channel */ + cl->dev = &pdev->dev; + cl->tx_done = xgene_hwmon_tx_done; + cl->tx_block = false; + cl->tx_tout = MBOX_OP_TIMEOUTMS; + cl->knows_txdone = false; + if (acpi_disabled) { + cl->rx_callback = xgene_hwmon_rx_cb; + ctx->mbox_chan = mbox_request_channel(cl, 0); + if (IS_ERR(ctx->mbox_chan)) { + dev_err(&pdev->dev, + "SLIMpro mailbox channel request failed\n"); + return -ENODEV; + } + } else { + struct acpi_pcct_hw_reduced *cppc_ss; + + if (device_property_read_u32(&pdev->dev, "pcc-channel", + &ctx->mbox_idx)) { + dev_err(&pdev->dev, "no pcc-channel property\n"); + return -ENODEV; + } + + cl->rx_callback = xgene_hwmon_pcc_rx_cb; + ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx); + if (IS_ERR(ctx->mbox_chan)) { + dev_err(&pdev->dev, + "PPC channel request failed\n"); + return -ENODEV; + } + + /* + * The PCC mailbox controller driver should + * have parsed the PCCT (global table of all + * PCC channels) and stored pointers to the + * subspace communication region in con_priv. + */ + cppc_ss = ctx->mbox_chan->con_priv; + if (!cppc_ss) { + dev_err(&pdev->dev, "PPC subspace not found\n"); + rc = -ENODEV; + goto out_mbox_free; + } + + if (!ctx->mbox_chan->mbox->txdone_irq) { + dev_err(&pdev->dev, "PCC IRQ not supported\n"); + rc = -ENODEV; + goto out_mbox_free; + } + + /* + * This is the shared communication region + * for the OS and Platform to communicate over. + */ + ctx->comm_base_addr = cppc_ss->base_address; + if (ctx->comm_base_addr) { + ctx->pcc_comm_addr = + acpi_os_ioremap(ctx->comm_base_addr, + cppc_ss->length); + } else { + dev_err(&pdev->dev, "Failed to get PCC comm region\n"); + rc = -ENODEV; + goto out_mbox_free; + } + + if (!ctx->pcc_comm_addr) { + dev_err(&pdev->dev, + "Failed to ioremap PCC comm region\n"); + rc = -ENOMEM; + goto out_mbox_free; + } + + /* + * cppc_ss->latency is just a Nominal value. In reality + * the remote processor could be much slower to reply. + * So add an arbitrary amount of wait on top of Nominal. + */ + ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency; + } + + spin_lock_init(&ctx->kfifo_lock); + mutex_init(&ctx->rd_mutex); + + rc = kfifo_alloc(&ctx->async_msg_fifo, + sizeof(struct slimpro_resp_msg) * ASYNC_MSG_FIFO_SIZE, + GFP_KERNEL); + if (rc) + goto out_mbox_free; + + INIT_WORK(&ctx->workq, xgene_hwmon_evt_work); + + ctx->hwmon_dev = devm_hwmon_device_register_with_groups(ctx->dev, + "apm_xgene", + ctx, + xgene_hwmon_groups); + if (IS_ERR(ctx->hwmon_dev)) { + dev_err(&pdev->dev, "Failed to register HW monitor device\n"); + rc = PTR_ERR(ctx->hwmon_dev); + goto out; + } + + dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n"); + + return rc; + +out: + kfifo_free(&ctx->async_msg_fifo); +out_mbox_free: + if (acpi_disabled) + mbox_free_channel(ctx->mbox_chan); + else + pcc_mbox_free_channel(ctx->mbox_chan); + + return rc; +} + +static int xgene_hwmon_remove(struct platform_device *pdev) +{ + struct xgene_hwmon_dev *ctx = platform_get_drvdata(pdev); + + kfifo_free(&ctx->async_msg_fifo); + if (acpi_disabled) + mbox_free_channel(ctx->mbox_chan); + else + pcc_mbox_free_channel(ctx->mbox_chan); + + return 0; +} + +#ifdef CONFIG_ACPI +static const struct acpi_device_id xgene_hwmon_acpi_match[] = { + {"APMC0D29", 0}, + {}, +}; +MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match); +#endif + +static const struct of_device_id xgene_hwmon_of_match[] = { + {.compatible = "apm,xgene-slimpro-hwmon"}, + {} +}; +MODULE_DEVICE_TABLE(of, xgene_hwmon_of_match); + +static struct platform_driver xgene_hwmon_driver __refdata = { + .probe = xgene_hwmon_probe, + .remove = xgene_hwmon_remove, + .driver = { + .name = "xgene-slimpro-hwmon", + .of_match_table = xgene_hwmon_of_match, + .acpi_match_table = ACPI_PTR(xgene_hwmon_acpi_match), + }, +}; +module_platform_driver(xgene_hwmon_driver); + +MODULE_DESCRIPTION("APM X-Gene SoC hardware monitor"); +MODULE_LICENSE("GPL"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] hwmon: xgene: Add hwmon driver 2016-07-12 0:30 ` [PATCH v2 2/3] hwmon: xgene: Add hwmon driver Hoan Tran @ 2016-07-16 16:35 ` Guenter Roeck 2016-07-18 21:30 ` Hoan Tran 0 siblings, 1 reply; 7+ messages in thread From: Guenter Roeck @ 2016-07-16 16:35 UTC (permalink / raw) To: Hoan Tran, Jean Delvare, Jonathan Corbet, Rob Herring Cc: Jassi Brar, Ashwin Chaugule, linux-hwmon, linux-doc, linux-kernel, linux-arm-kernel, devicetree, Duc Dang, lho On 07/11/2016 05:30 PM, Hoan Tran wrote: > This patch adds hardware temperature and power reading support for > APM X-Gene SoC using the mailbox communication interface. > > Signed-off-by: Hoan Tran <hotran@apm.com> > --- > Documentation/hwmon/xgene-hwmon | 30 ++ > drivers/hwmon/Kconfig | 7 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/xgene-hwmon.c | 772 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 810 insertions(+) > create mode 100644 Documentation/hwmon/xgene-hwmon > create mode 100644 drivers/hwmon/xgene-hwmon.c > > diff --git a/Documentation/hwmon/xgene-hwmon b/Documentation/hwmon/xgene-hwmon > new file mode 100644 > index 0000000..6ec50ed > --- /dev/null > +++ b/Documentation/hwmon/xgene-hwmon > @@ -0,0 +1,30 @@ > +Kernel driver xgene-hwmon > +======================== > + > +Supported chips: > + * APM X-Gene SoC > + > +Description > +----------- > + > +This driver adds hardware temperature and power reading support for > +APM X-Gene SoC using the mailbox communication interface. > +For device tree, it is the standard DT mailbox. > +For ACPI, it is the PCC mailbox. > + > +The following sensors are supported > + > + * Temperature > + - SoC on-die temperature in milli-degree C > + - Alarm when high/over temperature occurs > + * Power > + - CPU power in uW > + - IO power in uW > + > +sysfs-Interface > +--------------- > + > +temp0_input - SoC on-die temperature (milli-degree C) > +temp0_critical_alarm - An 1 would indicates on-die temperature exceeded threshold > +power0_input - CPU power in (uW) > +power1_input - IO power in (uW) > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index ff94007..55218c6 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1787,6 +1787,13 @@ config SENSORS_ULTRA45 > This driver provides support for the Ultra45 workstation environmental > sensors. > > +config SENSORS_XGENE > + tristate "APM X-Gene SoC hardware monitoring driver" > + depends on XGENE_SLIMPRO_MBOX || PCC > + help > + If you say yes here you get support for the temperature > + and power sensors for APM X-Gene SoC. > + > if ACPI > > comment "ACPI drivers" > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 2ef5b7c..a2460dd 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -162,6 +162,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o > obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o > obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o > obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o > +obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o > > obj-$(CONFIG_PMBUS) += pmbus/ > > diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c > new file mode 100644 > index 0000000..03917e3 > --- /dev/null > +++ b/drivers/hwmon/xgene-hwmon.c > @@ -0,0 +1,772 @@ > +/* > + * APM X-Gene SoC Hardware Monitoring Driver > + * > + * Copyright (c) 2016, Applied Micro Circuits Corporation > + * Author: Loc Ho <lho@apm.com> > + * Hoan Tran <hotran@apm.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + * > + * This driver provides the following features: > + * - Retrieve CPU total power (uW) > + * - Retrieve IO total power (uW) > + * - Retrieve SoC temperature (milli-degree C) and alarm > + */ > +#include <linux/acpi.h> > +#include <linux/dma-mapping.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/kfifo.h> > +#include <linux/interrupt.h> > +#include <linux/mailbox_controller.h> > +#include <linux/mailbox_client.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <acpi/acpi_io.h> > +#include <acpi/pcc.h> > + Please order include files alphabetically. > +/* SLIMpro message defines */ > +#define MSG_TYPE_DBG 0 > +#define MSG_TYPE_ERR 7 > +#define MSG_TYPE_PWRMGMT 9 Are those only used in this driver ? > +#define MSG_TYPE(v) (((v) & 0xF0000000) >> 28) > +#define MSG_TYPE_SET(v) (((v) << 28) & 0xF0000000) > +#define MSG_SUBTYPE(v) (((v) & 0x0F000000) >> 24) > +#define MSG_SUBTYPE_SET(v) (((v) << 24) & 0x0F000000) > + > +#define DBG_SUBTYPE_SENSOR_READ 4 > +#define SENSOR_RD_MSG 0x04FFE902 > +#define SENSOR_RD_EN_ADDR(a) ((a) & 0x000FFFFF) > +#define PMD_PWR_REG 0x20 > +#define PMD_PWR_MW_REG 0x26 > +#define SOC_PWR_REG 0x21 > +#define SOC_PWR_MW_REG 0x27 > +#define SOC_TEMP_REG 0x10 > + > +#define TEMP_NEGATIVE_BIT 8 > + > +#define PWRMGMT_SUBTYPE_TPC 1 > +#define TPC_ALARM 2 > +#define TPC_GET_ALARM 3 > +#define TPC_CMD(v) (((v) & 0x00FF0000) >> 16) > +#define TPC_CMD_SET(v) (((v) << 16) & 0x00FF0000) > +#define TPC_EN_MSG(hndl, cmd, type) \ > + (MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \ > + MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type) > + > +/* PCC defines */ > +#define PCC_SIGNATURE_MASK 0x50424300 > +#define PCCC_GENERATE_DB_INT BIT(15) > +#define PCCS_CMD_COMPLETE BIT(0) > +#define PCCS_SCI_DOORBEL BIT(1) > +#define PCCS_PLATFORM_NOTIFICATION BIT(3) > +/* > + * Arbitrary retries in case the remote processor is slow to respond > + * to PCC commands > + */ > +#define PCC_NUM_RETRIES 500 > + > +#define ASYNC_MSG_FIFO_SIZE 16 > +#define MBOX_OP_TIMEOUTMS 1000 > + > +#define WATT_TO_mWATT(x) ((x) * 1000) > +#define mWATT_TO_uWATT(x) ((x) * 1000) > +#define CELSIUS_TO_mCELSIUS(x) ((x) * 1000) > + > +#define to_xgene_hwmon_dev(cl) \ > + container_of(cl, struct xgene_hwmon_dev, mbox_client) > + > +struct slimpro_resp_msg { > + u32 msg; > + u32 param1; > + u32 param2; > +} __packed; > + > +struct xgene_hwmon_dev { > + struct device *dev; > + struct mbox_chan *mbox_chan; > + struct mbox_client mbox_client; > + int mbox_idx; > + > + spinlock_t kfifo_lock; > + struct mutex rd_mutex; > + struct completion rd_complete; > + int resp_pending; > + struct slimpro_resp_msg sync_msg; > + > + struct work_struct workq; > + struct kfifo_rec_ptr_1 async_msg_fifo; > + > + struct device *hwmon_dev; > + bool temp_critical_alarm; > + > + phys_addr_t comm_base_addr; > + void *pcc_comm_addr; > + u64 usecs_lat; > +}; > + > +/* > + * This function tests and clears a bitmask then returns its old value > + */ > +static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask) > +{ > + u16 ret, val; > + > + val = readw_relaxed(addr); > + ret = val & mask; > + val &= ~mask; > + writew_relaxed(val, addr); > + > + return ret; > +} > + > +static int xgene_hwmon_decode_temp(u32 val) The "hwmon" in the function names does not really add any value. I would suggest to drop it. > +{ > + unsigned long temp = val; > + > + /* Convert 9 bit signed temperature to integer */ > + if (test_and_clear_bit(TEMP_NEGATIVE_BIT, &temp)) > + return (temp - 256); > + else else after return is unnecessary. I would suggest to use sign_extend32(val, TEMP_NEGATIVE_BIT). > + return temp; > +} > + > +static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg) > +{ > + struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; > + void *ptr = generic_comm_base + 1; > + int rc, i; > + u16 val; > + > + mutex_lock(&ctx->rd_mutex); > + init_completion(&ctx->rd_complete); > + ctx->resp_pending = true; > + > + /* Write signature for subspace */ > + writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx, > + &generic_comm_base->signature); > + > + /* Write to the shared command region */ > + writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT, > + &generic_comm_base->command); > + > + /* Flip CMD COMPLETE bit */ > + val = readw_relaxed(&generic_comm_base->status); > + val &= ~PCCS_CMD_COMPLETE; > + writew_relaxed(val, &generic_comm_base->status); > + > + /* Copy the message to the PCC comm space */ > + for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++) > + writel_relaxed(msg[i], ptr + i * 4); > + > + /* Ring the doorbell */ > + rc = mbox_send_message(ctx->mbox_chan, msg); > + if (rc < 0) { > + dev_err(ctx->dev, "Mailbox send error %d\n", rc); > + goto err; > + } > + if (!wait_for_completion_timeout(&ctx->rd_complete, > + usecs_to_jiffies(ctx->usecs_lat))) { > + dev_err(ctx->dev, "Mailbox operation timed out\n"); > + rc = -ETIMEDOUT; > + goto err; > + } > + > + /* Check for invalid data */ > + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { > + rc = -EINVAL; > + goto err; > + } > + > + msg[0] = ctx->sync_msg.msg; > + msg[1] = ctx->sync_msg.param1; > + msg[2] = ctx->sync_msg.param2; > + > +err: > + mbox_chan_txdone(ctx->mbox_chan, 0); > + ctx->resp_pending = false; > + mutex_unlock(&ctx->rd_mutex); > + return rc; > +} > + > +static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg) > +{ > + int rc; > + > + mutex_lock(&ctx->rd_mutex); > + init_completion(&ctx->rd_complete); > + ctx->resp_pending = true; > + > + rc = mbox_send_message(ctx->mbox_chan, msg); > + if (rc < 0) { > + dev_err(ctx->dev, "Mailbox send error %d\n", rc); > + goto err; > + } > + > + if (!wait_for_completion_timeout(&ctx->rd_complete, > + msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) { > + dev_err(ctx->dev, "Mailbox operation timed out\n"); > + rc = -ETIMEDOUT; > + goto err; > + } > + > + /* Check for invalid data */ > + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { > + rc = -EINVAL; > + goto err; > + } > + > + msg[0] = ctx->sync_msg.msg; > + msg[1] = ctx->sync_msg.param1; > + msg[2] = ctx->sync_msg.param2; > + > +err: > + ctx->resp_pending = false; > + mutex_unlock(&ctx->rd_mutex); > + return rc; > +} > + > +static int xgene_hwmon_reg_map_rd(struct xgene_hwmon_dev *ctx, u32 addr, > + u32 *data) > +{ > + u32 msg[3]; > + int rc; > + > + msg[0] = SENSOR_RD_MSG; > + msg[1] = SENSOR_RD_EN_ADDR(addr); > + msg[2] = 0; > + > + if (acpi_disabled) > + rc = xgene_hwmon_rd(ctx, msg); > + else > + rc = xgene_hwmon_pcc_rd(ctx, msg); > + > + if (rc < 0) > + return rc; > + > + *data = msg[1]; > + > + return rc; > +} > + > +static int xgene_hwmon_get_notification_msg(struct xgene_hwmon_dev *ctx, > + u32 *amsg) > +{ > + u32 msg[3]; > + int rc; > + > + msg[0] = TPC_EN_MSG(PWRMGMT_SUBTYPE_TPC, TPC_GET_ALARM, 0); > + msg[1] = 0; > + msg[2] = 0; > + > + rc = xgene_hwmon_pcc_rd(ctx, msg); > + if (rc < 0) > + return rc; > + > + amsg[0] = msg[0]; > + amsg[1] = msg[1]; > + amsg[2] = msg[2]; > + > + return rc; > +} > + > +static int xgene_hwmon_get_cpu_pwr(struct xgene_hwmon_dev *ctx, u32 *val) > +{ > + u32 watt, mwatt; > + int rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_REG, &watt); > + if (rc < 0) > + return rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, &mwatt); > + if (rc < 0) > + return rc; > + > + *val = WATT_TO_mWATT(watt) + mwatt; > + return 0; > +} > + > +static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val) > +{ > + u32 watt, mwatt; > + int rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, &watt); > + if (rc < 0) > + return rc; > + > + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_MW_REG, &mwatt); > + if (rc < 0) > + return rc; > + > + *val = WATT_TO_mWATT(watt) + mwatt; > + return 0; > +} > + > +static int xgene_hwmon_get_temp(struct xgene_hwmon_dev *ctx, u32 *val) > +{ > + return xgene_hwmon_reg_map_rd(ctx, SOC_TEMP_REG, val); > +} > + > +/* > + * Sensor temperature/power functions > + */ > +static ssize_t xgene_hwmon_show_temp(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + int rc, temp; > + u32 val; > + > + rc = xgene_hwmon_get_temp(ctx, &val); > + if (rc < 0) > + return rc; > + > + temp = xgene_hwmon_decode_temp(val); > + > + return snprintf(buf, PAGE_SIZE, "%d\n", CELSIUS_TO_mCELSIUS(temp)); > +} > + > +static ssize_t xgene_hwmon_show_temp_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "SoC Temperature\n"); > +} > + > +static ssize_t > +xgene_hwmon_show_temp_critical_alarm(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + > + return snprintf(buf, PAGE_SIZE, "%d\n", ctx->temp_critical_alarm); > +} > + > + > +static ssize_t xgene_hwmon_show_cpu_pwr_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "CPU power\n"); > +} > + > +static ssize_t xgene_hwmon_show_io_pwr_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "IO power\n"); > +} > + > +static ssize_t xgene_hwmon_show_cpu_pwr(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + u32 val; > + int rc; > + > + rc = xgene_hwmon_get_cpu_pwr(ctx, &val); > + if (rc < 0) > + return rc; > + > + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); > +} > + > +static ssize_t xgene_hwmon_show_io_pwr(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); > + u32 val; > + int rc; > + > + rc = xgene_hwmon_get_io_pwr(ctx, &val); > + if (rc < 0) > + return rc; > + > + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); > +} > + > +static SENSOR_DEVICE_ATTR(temp0_label, S_IRUGO, xgene_hwmon_show_temp_label, > + NULL, 0); > +static SENSOR_DEVICE_ATTR(temp0_input, S_IRUGO, xgene_hwmon_show_temp, > + NULL, 0); > +static SENSOR_DEVICE_ATTR(temp0_critical_alarm, S_IRUGO, > + xgene_hwmon_show_temp_critical_alarm, NULL, 0); > + > +static SENSOR_DEVICE_ATTR(power0_label, S_IRUGO, > + xgene_hwmon_show_cpu_pwr_label, NULL, 0); > +static SENSOR_DEVICE_ATTR(power0_input, S_IRUGO, xgene_hwmon_show_cpu_pwr, > + NULL, 0); > + > +static SENSOR_DEVICE_ATTR(power1_label, S_IRUGO, > + xgene_hwmon_show_io_pwr_label, NULL, 1); > +static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, xgene_hwmon_show_io_pwr, > + NULL, 1); > + Temperature and power attributes start with index 1. You don't use the index value in SENSOR_DEVICE_ATTR(), so DEVCIE_ATTR_RO() might be a better choice. > +static struct attribute *xgene_hwmon_attrs[] = { > + &sensor_dev_attr_temp0_label.dev_attr.attr, > + &sensor_dev_attr_temp0_input.dev_attr.attr, > + &sensor_dev_attr_temp0_critical_alarm.dev_attr.attr, > + &sensor_dev_attr_power0_label.dev_attr.attr, > + &sensor_dev_attr_power0_input.dev_attr.attr, > + &sensor_dev_attr_power1_label.dev_attr.attr, > + &sensor_dev_attr_power1_input.dev_attr.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(xgene_hwmon); > + > +static int xgene_hwmon_tpc_alarm(struct xgene_hwmon_dev *ctx, > + struct slimpro_resp_msg *amsg) > +{ > + char name[40]; > + > + ctx->temp_critical_alarm = amsg->param2 ? true : false; You could use !!amsg->param2, or simply rely on the compiler and C standard to auto-convert the integer to boolean. > + snprintf(name, sizeof(name), "temp0_critical_alarm"); > + sysfs_notify(&ctx->dev->kobj, NULL, name); Why not just use "temp0_critical_alarm" (or rather "temp1_critical_alarm") as parameter to sysfs_notify() ? > + > + return 0; > +} > + > +static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx, > + struct slimpro_resp_msg *amsg) > +{ > + if ((MSG_SUBTYPE(amsg->msg) == PWRMGMT_SUBTYPE_TPC) && > + (TPC_CMD(amsg->msg) == TPC_ALARM)) > + xgene_hwmon_tpc_alarm(ctx, amsg); > +} > + > +/* > + * This function is called to process async work queue > + */ > +static void xgene_hwmon_evt_work(struct work_struct *work) > +{ > + struct slimpro_resp_msg amsg; > + struct xgene_hwmon_dev *ctx; > + int ret; > + > + ctx = container_of(work, struct xgene_hwmon_dev, workq); > + while (kfifo_out_spinlocked(&ctx->async_msg_fifo, &amsg, > + sizeof(struct slimpro_resp_msg), > + &ctx->kfifo_lock)) { > + /* > + * If PCC, send a consumer command to Platform to get info > + * If Slimpro Mailbox, get message from specific FIFO > + */ > + if (!acpi_disabled) { > + ret = xgene_hwmon_get_notification_msg(ctx, > + (u32 *)&amsg); > + if (ret < 0) > + continue; > + } > + > + if (MSG_TYPE(amsg.msg) == MSG_TYPE_PWRMGMT) > + xgene_hwmon_process_pwrmsg(ctx, &amsg); > + } > +} > + > +/* > + * This function is called when the SLIMpro Mailbox received a message > + */ > +static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg) > +{ > + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); > + struct slimpro_resp_msg amsg; > + > + /* > + * Response message format: > + * msg[0] is the return code of the operation > + * msg[1] is the first parameter word > + * msg[2] is the second parameter word > + * > + * As message only supports dword size, just assign it. > + */ > + > + /* Check for sync query */ > + if (ctx->resp_pending && > + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && > + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && > + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && > + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { > + ctx->sync_msg.msg = ((u32 *) msg)[0]; > + ctx->sync_msg.param1 = ((u32 *) msg)[1]; > + ctx->sync_msg.param2 = ((u32 *) msg)[2]; > + > + /* Operation waiting for response */ > + complete(&ctx->rd_complete); > + > + return; > + } > + > + amsg.msg = ((u32 *) msg)[0]; > + amsg.param1 = ((u32 *) msg)[1]; > + amsg.param2 = ((u32 *) msg)[2]; > + > + /* Enqueue to the FIFO */ > + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, > + sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock); > + /* Schedule the bottom handler */ > + schedule_work(&ctx->workq); > +} > + > +/* > + * This function is called when the PCC Mailbox received a message > + */ > +static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg) > +{ > + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); > + struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; > + struct slimpro_resp_msg amsg; > + > + msg = generic_comm_base + 1; > + /* Check if platform sends interrupt */ > + if (!xgene_word_tst_and_clr(&generic_comm_base->status, > + PCCS_SCI_DOORBEL)) > + return; > + > + /* > + * Response message format: > + * msg[0] is the return code of the operation > + * msg[1] is the first parameter word > + * msg[2] is the second parameter word > + * > + * As message only supports dword size, just assign it. > + */ > + > + /* Check for sync query */ > + if (ctx->resp_pending && > + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && > + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || > + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && > + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && > + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { > + > + /* Check if platform completes command */ > + if (xgene_word_tst_and_clr(&generic_comm_base->status, > + PCCS_CMD_COMPLETE)) { > + ctx->sync_msg.msg = ((u32 *) msg)[0]; > + ctx->sync_msg.param1 = ((u32 *) msg)[1]; > + ctx->sync_msg.param2 = ((u32 *) msg)[2]; > + > + /* Operation waiting for response */ > + complete(&ctx->rd_complete); > + > + return; > + } > + } > + > + /* > + * Platform notifies interrupt to OSPM. > + * OPSM schedules a consumer command to get this information > + * in a workqueue. Platform must wait until OSPM has issued > + * a consumer command that serves this notification. > + */ > + > + /* Enqueue to the FIFO */ > + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, > + sizeof(struct slimpro_resp_msg), &ctx->kfifo_lock); > + /* Schedule the bottom handler */ > + schedule_work(&ctx->workq); > +} > + > +static void xgene_hwmon_tx_done(struct mbox_client *cl, void *msg, int ret) > +{ > + if (ret) { > + dev_dbg(cl->dev, "TX did not complete: CMD sent:%x, ret:%d\n", > + *(u16 *) msg, ret); > + } else { > + dev_dbg(cl->dev, "TX completed. CMD sent:%x, ret:%d\n", > + *(u16 *) msg, ret); Please run your patch through checkoatch --strict. I would like to see at least the following messages resolved. "No space is necessary after a cast" "Blank lines aren't necessary after an open brace '{'" "Please don't use multiple blank lines" "Alignment should match open parenthesis" "line over 80 characters" (with precedence over continuation line alignment) > + } > +} > + > +static int xgene_hwmon_probe(struct platform_device *pdev) > +{ > + struct xgene_hwmon_dev *ctx; > + struct mbox_client *cl; > + int rc; > + > + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + > + ctx->dev = &pdev->dev; > + platform_set_drvdata(pdev, ctx); > + cl = &ctx->mbox_client; > + > + /* Request mailbox channel */ > + cl->dev = &pdev->dev; > + cl->tx_done = xgene_hwmon_tx_done; > + cl->tx_block = false; > + cl->tx_tout = MBOX_OP_TIMEOUTMS; > + cl->knows_txdone = false; > + if (acpi_disabled) { > + cl->rx_callback = xgene_hwmon_rx_cb; > + ctx->mbox_chan = mbox_request_channel(cl, 0); > + if (IS_ERR(ctx->mbox_chan)) { > + dev_err(&pdev->dev, > + "SLIMpro mailbox channel request failed\n"); > + return -ENODEV; > + } > + } else { > + struct acpi_pcct_hw_reduced *cppc_ss; > + > + if (device_property_read_u32(&pdev->dev, "pcc-channel", > + &ctx->mbox_idx)) { > + dev_err(&pdev->dev, "no pcc-channel property\n"); > + return -ENODEV; > + } > + > + cl->rx_callback = xgene_hwmon_pcc_rx_cb; > + ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx); > + if (IS_ERR(ctx->mbox_chan)) { > + dev_err(&pdev->dev, > + "PPC channel request failed\n"); > + return -ENODEV; > + } > + > + /* > + * The PCC mailbox controller driver should > + * have parsed the PCCT (global table of all > + * PCC channels) and stored pointers to the > + * subspace communication region in con_priv. > + */ > + cppc_ss = ctx->mbox_chan->con_priv; > + if (!cppc_ss) { > + dev_err(&pdev->dev, "PPC subspace not found\n"); > + rc = -ENODEV; > + goto out_mbox_free; > + } > + > + if (!ctx->mbox_chan->mbox->txdone_irq) { > + dev_err(&pdev->dev, "PCC IRQ not supported\n"); > + rc = -ENODEV; > + goto out_mbox_free; > + } > + > + /* > + * This is the shared communication region > + * for the OS and Platform to communicate over. > + */ > + ctx->comm_base_addr = cppc_ss->base_address; > + if (ctx->comm_base_addr) { > + ctx->pcc_comm_addr = > + acpi_os_ioremap(ctx->comm_base_addr, > + cppc_ss->length); > + } else { > + dev_err(&pdev->dev, "Failed to get PCC comm region\n"); > + rc = -ENODEV; > + goto out_mbox_free; > + } > + > + if (!ctx->pcc_comm_addr) { > + dev_err(&pdev->dev, > + "Failed to ioremap PCC comm region\n"); > + rc = -ENOMEM; > + goto out_mbox_free; > + } > + > + /* > + * cppc_ss->latency is just a Nominal value. In reality > + * the remote processor could be much slower to reply. > + * So add an arbitrary amount of wait on top of Nominal. > + */ > + ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency; > + } > + > + spin_lock_init(&ctx->kfifo_lock); > + mutex_init(&ctx->rd_mutex); > + > + rc = kfifo_alloc(&ctx->async_msg_fifo, > + sizeof(struct slimpro_resp_msg) * ASYNC_MSG_FIFO_SIZE, > + GFP_KERNEL); > + if (rc) > + goto out_mbox_free; > + > + INIT_WORK(&ctx->workq, xgene_hwmon_evt_work); > + > + ctx->hwmon_dev = devm_hwmon_device_register_with_groups(ctx->dev, > + "apm_xgene", > + ctx, > + xgene_hwmon_groups); Using the devm_ function here would, on remove, result in the mailbox and kfifo being removed before the hwmon driver is removed. This might result in race conditions. You have two options: Use devm_add_action() to remove those in sequence, or use hwmon_device_register_with_groups() and remove the hwmon device explicitly (and first) in the remove function. Note that you don't have to store hwmon_dev in ctx if you use devm_add_action() and devm_hwmon_device_register_with_groups(). > + if (IS_ERR(ctx->hwmon_dev)) { > + dev_err(&pdev->dev, "Failed to register HW monitor device\n"); > + rc = PTR_ERR(ctx->hwmon_dev); > + goto out; > + } > + > + dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n"); > + > + return rc; > + > +out: > + kfifo_free(&ctx->async_msg_fifo); > +out_mbox_free: > + if (acpi_disabled) > + mbox_free_channel(ctx->mbox_chan); > + else > + pcc_mbox_free_channel(ctx->mbox_chan); > + > + return rc; > +} > + > +static int xgene_hwmon_remove(struct platform_device *pdev) > +{ > + struct xgene_hwmon_dev *ctx = platform_get_drvdata(pdev); > + > + kfifo_free(&ctx->async_msg_fifo); > + if (acpi_disabled) > + mbox_free_channel(ctx->mbox_chan); > + else > + pcc_mbox_free_channel(ctx->mbox_chan); > + > + return 0; > +} > + > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id xgene_hwmon_acpi_match[] = { > + {"APMC0D29", 0}, > + {}, > +}; > +MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match); > +#endif > + > +static const struct of_device_id xgene_hwmon_of_match[] = { > + {.compatible = "apm,xgene-slimpro-hwmon"}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, xgene_hwmon_of_match); > + > +static struct platform_driver xgene_hwmon_driver __refdata = { > + .probe = xgene_hwmon_probe, > + .remove = xgene_hwmon_remove, > + .driver = { > + .name = "xgene-slimpro-hwmon", > + .of_match_table = xgene_hwmon_of_match, > + .acpi_match_table = ACPI_PTR(xgene_hwmon_acpi_match), > + }, > +}; > +module_platform_driver(xgene_hwmon_driver); > + > +MODULE_DESCRIPTION("APM X-Gene SoC hardware monitor"); > +MODULE_LICENSE("GPL"); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] hwmon: xgene: Add hwmon driver 2016-07-16 16:35 ` Guenter Roeck @ 2016-07-18 21:30 ` Hoan Tran 0 siblings, 0 replies; 7+ messages in thread From: Hoan Tran @ 2016-07-18 21:30 UTC (permalink / raw) To: Guenter Roeck Cc: Jean Delvare, Jonathan Corbet, Rob Herring, Jassi Brar, Ashwin Chaugule, linux-hwmon, linux-doc, lkml, linux-arm-kernel, Devicetree List, Duc Dang, Loc Ho Hi Guenter, On Sat, Jul 16, 2016 at 9:35 AM, Guenter Roeck <linux@roeck-us.net> wrote: > On 07/11/2016 05:30 PM, Hoan Tran wrote: >> >> This patch adds hardware temperature and power reading support for >> APM X-Gene SoC using the mailbox communication interface. >> >> Signed-off-by: Hoan Tran <hotran@apm.com> >> --- >> Documentation/hwmon/xgene-hwmon | 30 ++ >> drivers/hwmon/Kconfig | 7 + >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/xgene-hwmon.c | 772 >> ++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 810 insertions(+) >> create mode 100644 Documentation/hwmon/xgene-hwmon >> create mode 100644 drivers/hwmon/xgene-hwmon.c >> >> diff --git a/Documentation/hwmon/xgene-hwmon >> b/Documentation/hwmon/xgene-hwmon >> new file mode 100644 >> index 0000000..6ec50ed >> --- /dev/null >> +++ b/Documentation/hwmon/xgene-hwmon >> @@ -0,0 +1,30 @@ >> +Kernel driver xgene-hwmon >> +======================== >> + >> +Supported chips: >> + * APM X-Gene SoC >> + >> +Description >> +----------- >> + >> +This driver adds hardware temperature and power reading support for >> +APM X-Gene SoC using the mailbox communication interface. >> +For device tree, it is the standard DT mailbox. >> +For ACPI, it is the PCC mailbox. >> + >> +The following sensors are supported >> + >> + * Temperature >> + - SoC on-die temperature in milli-degree C >> + - Alarm when high/over temperature occurs >> + * Power >> + - CPU power in uW >> + - IO power in uW >> + >> +sysfs-Interface >> +--------------- >> + >> +temp0_input - SoC on-die temperature (milli-degree C) >> +temp0_critical_alarm - An 1 would indicates on-die temperature exceeded >> threshold >> +power0_input - CPU power in (uW) >> +power1_input - IO power in (uW) >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index ff94007..55218c6 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1787,6 +1787,13 @@ config SENSORS_ULTRA45 >> This driver provides support for the Ultra45 workstation >> environmental >> sensors. >> >> +config SENSORS_XGENE >> + tristate "APM X-Gene SoC hardware monitoring driver" >> + depends on XGENE_SLIMPRO_MBOX || PCC >> + help >> + If you say yes here you get support for the temperature >> + and power sensors for APM X-Gene SoC. >> + >> if ACPI >> >> comment "ACPI drivers" >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index 2ef5b7c..a2460dd 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -162,6 +162,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o >> obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o >> obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o >> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o >> +obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o >> >> obj-$(CONFIG_PMBUS) += pmbus/ >> >> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c >> new file mode 100644 >> index 0000000..03917e3 >> --- /dev/null >> +++ b/drivers/hwmon/xgene-hwmon.c >> @@ -0,0 +1,772 @@ >> +/* >> + * APM X-Gene SoC Hardware Monitoring Driver >> + * >> + * Copyright (c) 2016, Applied Micro Circuits Corporation >> + * Author: Loc Ho <lho@apm.com> >> + * Hoan Tran <hotran@apm.com> >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >> + * >> + * This driver provides the following features: >> + * - Retrieve CPU total power (uW) >> + * - Retrieve IO total power (uW) >> + * - Retrieve SoC temperature (milli-degree C) and alarm >> + */ >> +#include <linux/acpi.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/hwmon.h> >> +#include <linux/hwmon-sysfs.h> >> +#include <linux/kfifo.h> >> +#include <linux/interrupt.h> >> +#include <linux/mailbox_controller.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <acpi/acpi_io.h> >> +#include <acpi/pcc.h> >> + > > Please order include files alphabetically. > >> +/* SLIMpro message defines */ >> +#define MSG_TYPE_DBG 0 >> +#define MSG_TYPE_ERR 7 >> +#define MSG_TYPE_PWRMGMT 9 > > > Are those only used in this driver ? MSG_TYPE_DBG is used by another driver, i2c_xgene_slimpro. Others are only used by this driver. > > >> +#define MSG_TYPE(v) (((v) & 0xF0000000) >> 28) >> +#define MSG_TYPE_SET(v) (((v) << 28) & 0xF0000000) >> +#define MSG_SUBTYPE(v) (((v) & 0x0F000000) >> 24) >> +#define MSG_SUBTYPE_SET(v) (((v) << 24) & 0x0F000000) >> + >> +#define DBG_SUBTYPE_SENSOR_READ 4 >> +#define SENSOR_RD_MSG 0x04FFE902 >> +#define SENSOR_RD_EN_ADDR(a) ((a) & 0x000FFFFF) >> +#define PMD_PWR_REG 0x20 >> +#define PMD_PWR_MW_REG 0x26 >> +#define SOC_PWR_REG 0x21 >> +#define SOC_PWR_MW_REG 0x27 >> +#define SOC_TEMP_REG 0x10 >> + >> +#define TEMP_NEGATIVE_BIT 8 >> + >> +#define PWRMGMT_SUBTYPE_TPC 1 >> +#define TPC_ALARM 2 >> +#define TPC_GET_ALARM 3 >> +#define TPC_CMD(v) (((v) & 0x00FF0000) >> 16) >> +#define TPC_CMD_SET(v) (((v) << 16) & 0x00FF0000) >> +#define TPC_EN_MSG(hndl, cmd, type) \ >> + (MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \ >> + MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type) >> + >> +/* PCC defines */ >> +#define PCC_SIGNATURE_MASK 0x50424300 >> +#define PCCC_GENERATE_DB_INT BIT(15) >> +#define PCCS_CMD_COMPLETE BIT(0) >> +#define PCCS_SCI_DOORBEL BIT(1) >> +#define PCCS_PLATFORM_NOTIFICATION BIT(3) >> +/* >> + * Arbitrary retries in case the remote processor is slow to respond >> + * to PCC commands >> + */ >> +#define PCC_NUM_RETRIES 500 >> + >> +#define ASYNC_MSG_FIFO_SIZE 16 >> +#define MBOX_OP_TIMEOUTMS 1000 >> + >> +#define WATT_TO_mWATT(x) ((x) * 1000) >> +#define mWATT_TO_uWATT(x) ((x) * 1000) >> +#define CELSIUS_TO_mCELSIUS(x) ((x) * 1000) >> + >> +#define to_xgene_hwmon_dev(cl) \ >> + container_of(cl, struct xgene_hwmon_dev, mbox_client) >> + >> +struct slimpro_resp_msg { >> + u32 msg; >> + u32 param1; >> + u32 param2; >> +} __packed; >> + >> +struct xgene_hwmon_dev { >> + struct device *dev; >> + struct mbox_chan *mbox_chan; >> + struct mbox_client mbox_client; >> + int mbox_idx; >> + >> + spinlock_t kfifo_lock; >> + struct mutex rd_mutex; >> + struct completion rd_complete; >> + int resp_pending; >> + struct slimpro_resp_msg sync_msg; >> + >> + struct work_struct workq; >> + struct kfifo_rec_ptr_1 async_msg_fifo; >> + >> + struct device *hwmon_dev; >> + bool temp_critical_alarm; >> + >> + phys_addr_t comm_base_addr; >> + void *pcc_comm_addr; >> + u64 usecs_lat; >> +}; >> + >> +/* >> + * This function tests and clears a bitmask then returns its old value >> + */ >> +static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask) >> +{ >> + u16 ret, val; >> + >> + val = readw_relaxed(addr); >> + ret = val & mask; >> + val &= ~mask; >> + writew_relaxed(val, addr); >> + >> + return ret; >> +} >> + >> +static int xgene_hwmon_decode_temp(u32 val) > > > The "hwmon" in the function names does not really add any value. > I would suggest to drop it. > >> +{ >> + unsigned long temp = val; >> + >> + /* Convert 9 bit signed temperature to integer */ >> + if (test_and_clear_bit(TEMP_NEGATIVE_BIT, &temp)) >> + return (temp - 256); >> + else > > > else after return is unnecessary. > I would suggest to use sign_extend32(val, TEMP_NEGATIVE_BIT). That's a good suggestion. I'll use sign_extend32(). > > >> + return temp; >> +} >> + >> +static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg) >> +{ >> + struct acpi_pcct_shared_memory *generic_comm_base = >> ctx->pcc_comm_addr; >> + void *ptr = generic_comm_base + 1; >> + int rc, i; >> + u16 val; >> + >> + mutex_lock(&ctx->rd_mutex); >> + init_completion(&ctx->rd_complete); >> + ctx->resp_pending = true; >> + >> + /* Write signature for subspace */ >> + writel_relaxed(PCC_SIGNATURE_MASK | ctx->mbox_idx, >> + &generic_comm_base->signature); >> + >> + /* Write to the shared command region */ >> + writew_relaxed(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT, >> + &generic_comm_base->command); >> + >> + /* Flip CMD COMPLETE bit */ >> + val = readw_relaxed(&generic_comm_base->status); >> + val &= ~PCCS_CMD_COMPLETE; >> + writew_relaxed(val, &generic_comm_base->status); >> + >> + /* Copy the message to the PCC comm space */ >> + for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++) >> + writel_relaxed(msg[i], ptr + i * 4); >> + >> + /* Ring the doorbell */ >> + rc = mbox_send_message(ctx->mbox_chan, msg); >> + if (rc < 0) { >> + dev_err(ctx->dev, "Mailbox send error %d\n", rc); >> + goto err; >> + } >> + if (!wait_for_completion_timeout(&ctx->rd_complete, >> + >> usecs_to_jiffies(ctx->usecs_lat))) { >> + dev_err(ctx->dev, "Mailbox operation timed out\n"); >> + rc = -ETIMEDOUT; >> + goto err; >> + } >> + >> + /* Check for invalid data */ >> + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { >> + rc = -EINVAL; >> + goto err; >> + } >> + >> + msg[0] = ctx->sync_msg.msg; >> + msg[1] = ctx->sync_msg.param1; >> + msg[2] = ctx->sync_msg.param2; >> + >> +err: >> + mbox_chan_txdone(ctx->mbox_chan, 0); >> + ctx->resp_pending = false; >> + mutex_unlock(&ctx->rd_mutex); >> + return rc; >> +} >> + >> +static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg) >> +{ >> + int rc; >> + >> + mutex_lock(&ctx->rd_mutex); >> + init_completion(&ctx->rd_complete); >> + ctx->resp_pending = true; >> + >> + rc = mbox_send_message(ctx->mbox_chan, msg); >> + if (rc < 0) { >> + dev_err(ctx->dev, "Mailbox send error %d\n", rc); >> + goto err; >> + } >> + >> + if (!wait_for_completion_timeout(&ctx->rd_complete, >> + >> msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) { >> + dev_err(ctx->dev, "Mailbox operation timed out\n"); >> + rc = -ETIMEDOUT; >> + goto err; >> + } >> + >> + /* Check for invalid data */ >> + if (MSG_TYPE(ctx->sync_msg.msg) == MSG_TYPE_ERR) { >> + rc = -EINVAL; >> + goto err; >> + } >> + >> + msg[0] = ctx->sync_msg.msg; >> + msg[1] = ctx->sync_msg.param1; >> + msg[2] = ctx->sync_msg.param2; >> + >> +err: >> + ctx->resp_pending = false; >> + mutex_unlock(&ctx->rd_mutex); >> + return rc; >> +} >> + >> +static int xgene_hwmon_reg_map_rd(struct xgene_hwmon_dev *ctx, u32 addr, >> + u32 *data) >> +{ >> + u32 msg[3]; >> + int rc; >> + >> + msg[0] = SENSOR_RD_MSG; >> + msg[1] = SENSOR_RD_EN_ADDR(addr); >> + msg[2] = 0; >> + >> + if (acpi_disabled) >> + rc = xgene_hwmon_rd(ctx, msg); >> + else >> + rc = xgene_hwmon_pcc_rd(ctx, msg); >> + >> + if (rc < 0) >> + return rc; >> + >> + *data = msg[1]; >> + >> + return rc; >> +} >> + >> +static int xgene_hwmon_get_notification_msg(struct xgene_hwmon_dev *ctx, >> + u32 *amsg) >> +{ >> + u32 msg[3]; >> + int rc; >> + >> + msg[0] = TPC_EN_MSG(PWRMGMT_SUBTYPE_TPC, TPC_GET_ALARM, 0); >> + msg[1] = 0; >> + msg[2] = 0; >> + >> + rc = xgene_hwmon_pcc_rd(ctx, msg); >> + if (rc < 0) >> + return rc; >> + >> + amsg[0] = msg[0]; >> + amsg[1] = msg[1]; >> + amsg[2] = msg[2]; >> + >> + return rc; >> +} >> + >> +static int xgene_hwmon_get_cpu_pwr(struct xgene_hwmon_dev *ctx, u32 *val) >> +{ >> + u32 watt, mwatt; >> + int rc; >> + >> + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_REG, &watt); >> + if (rc < 0) >> + return rc; >> + >> + rc = xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, &mwatt); >> + if (rc < 0) >> + return rc; >> + >> + *val = WATT_TO_mWATT(watt) + mwatt; >> + return 0; >> +} >> + >> +static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val) >> +{ >> + u32 watt, mwatt; >> + int rc; >> + >> + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, &watt); >> + if (rc < 0) >> + return rc; >> + >> + rc = xgene_hwmon_reg_map_rd(ctx, SOC_PWR_MW_REG, &mwatt); >> + if (rc < 0) >> + return rc; >> + >> + *val = WATT_TO_mWATT(watt) + mwatt; >> + return 0; >> +} >> + >> +static int xgene_hwmon_get_temp(struct xgene_hwmon_dev *ctx, u32 *val) >> +{ >> + return xgene_hwmon_reg_map_rd(ctx, SOC_TEMP_REG, val); >> +} >> + >> +/* >> + * Sensor temperature/power functions >> + */ >> +static ssize_t xgene_hwmon_show_temp(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); >> + int rc, temp; >> + u32 val; >> + >> + rc = xgene_hwmon_get_temp(ctx, &val); >> + if (rc < 0) >> + return rc; >> + >> + temp = xgene_hwmon_decode_temp(val); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> CELSIUS_TO_mCELSIUS(temp)); >> +} >> + >> +static ssize_t xgene_hwmon_show_temp_label(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + return snprintf(buf, PAGE_SIZE, "SoC Temperature\n"); >> +} >> + >> +static ssize_t >> +xgene_hwmon_show_temp_critical_alarm(struct device *dev, >> + struct device_attribute *devattr, >> + char *buf) >> +{ >> + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", ctx->temp_critical_alarm); >> +} >> + >> + >> +static ssize_t xgene_hwmon_show_cpu_pwr_label(struct device *dev, >> + struct device_attribute >> *attr, >> + char *buf) >> +{ >> + return snprintf(buf, PAGE_SIZE, "CPU power\n"); >> +} >> + >> +static ssize_t xgene_hwmon_show_io_pwr_label(struct device *dev, >> + struct device_attribute >> *attr, >> + char *buf) >> +{ >> + return snprintf(buf, PAGE_SIZE, "IO power\n"); >> +} >> + >> +static ssize_t xgene_hwmon_show_cpu_pwr(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); >> + u32 val; >> + int rc; >> + >> + rc = xgene_hwmon_get_cpu_pwr(ctx, &val); >> + if (rc < 0) >> + return rc; >> + >> + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); >> +} >> + >> +static ssize_t xgene_hwmon_show_io_pwr(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev); >> + u32 val; >> + int rc; >> + >> + rc = xgene_hwmon_get_io_pwr(ctx, &val); >> + if (rc < 0) >> + return rc; >> + >> + return snprintf(buf, PAGE_SIZE, "%u\n", mWATT_TO_uWATT(val)); >> +} >> + >> +static SENSOR_DEVICE_ATTR(temp0_label, S_IRUGO, >> xgene_hwmon_show_temp_label, >> + NULL, 0); >> +static SENSOR_DEVICE_ATTR(temp0_input, S_IRUGO, xgene_hwmon_show_temp, >> + NULL, 0); >> +static SENSOR_DEVICE_ATTR(temp0_critical_alarm, S_IRUGO, >> + xgene_hwmon_show_temp_critical_alarm, NULL, 0); >> + >> +static SENSOR_DEVICE_ATTR(power0_label, S_IRUGO, >> + xgene_hwmon_show_cpu_pwr_label, NULL, 0); >> +static SENSOR_DEVICE_ATTR(power0_input, S_IRUGO, >> xgene_hwmon_show_cpu_pwr, >> + NULL, 0); >> + >> +static SENSOR_DEVICE_ATTR(power1_label, S_IRUGO, >> + xgene_hwmon_show_io_pwr_label, NULL, 1); >> +static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, xgene_hwmon_show_io_pwr, >> + NULL, 1); >> + > > > Temperature and power attributes start with index 1. You don't use the index > value in > SENSOR_DEVICE_ATTR(), so DEVCIE_ATTR_RO() might be a better choice. Yes, I'll use DEVICE_ATTR_RO() and start with index 1. > >> +static struct attribute *xgene_hwmon_attrs[] = { >> + &sensor_dev_attr_temp0_label.dev_attr.attr, >> + &sensor_dev_attr_temp0_input.dev_attr.attr, >> + &sensor_dev_attr_temp0_critical_alarm.dev_attr.attr, >> + &sensor_dev_attr_power0_label.dev_attr.attr, >> + &sensor_dev_attr_power0_input.dev_attr.attr, >> + &sensor_dev_attr_power1_label.dev_attr.attr, >> + &sensor_dev_attr_power1_input.dev_attr.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(xgene_hwmon); >> + >> +static int xgene_hwmon_tpc_alarm(struct xgene_hwmon_dev *ctx, >> + struct slimpro_resp_msg *amsg) >> +{ >> + char name[40]; >> + >> + ctx->temp_critical_alarm = amsg->param2 ? true : false; > > > You could use !!amsg->param2, or simply rely on the compiler and C standard > to auto-convert the integer to boolean. > >> + snprintf(name, sizeof(name), "temp0_critical_alarm"); >> + sysfs_notify(&ctx->dev->kobj, NULL, name); > > > Why not just use "temp0_critical_alarm" (or rather "temp1_critical_alarm") > as parameter to sysfs_notify() ? > > >> + >> + return 0; >> +} >> + >> +static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx, >> + struct slimpro_resp_msg *amsg) >> +{ >> + if ((MSG_SUBTYPE(amsg->msg) == PWRMGMT_SUBTYPE_TPC) && >> + (TPC_CMD(amsg->msg) == TPC_ALARM)) >> + xgene_hwmon_tpc_alarm(ctx, amsg); >> +} >> + >> +/* >> + * This function is called to process async work queue >> + */ >> +static void xgene_hwmon_evt_work(struct work_struct *work) >> +{ >> + struct slimpro_resp_msg amsg; >> + struct xgene_hwmon_dev *ctx; >> + int ret; >> + >> + ctx = container_of(work, struct xgene_hwmon_dev, workq); >> + while (kfifo_out_spinlocked(&ctx->async_msg_fifo, &amsg, >> + sizeof(struct slimpro_resp_msg), >> + &ctx->kfifo_lock)) { >> + /* >> + * If PCC, send a consumer command to Platform to get info >> + * If Slimpro Mailbox, get message from specific FIFO >> + */ >> + if (!acpi_disabled) { >> + ret = xgene_hwmon_get_notification_msg(ctx, >> + (u32 >> *)&amsg); >> + if (ret < 0) >> + continue; >> + } >> + >> + if (MSG_TYPE(amsg.msg) == MSG_TYPE_PWRMGMT) >> + xgene_hwmon_process_pwrmsg(ctx, &amsg); >> + } >> +} >> + >> +/* >> + * This function is called when the SLIMpro Mailbox received a message >> + */ >> +static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg) >> +{ >> + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); >> + struct slimpro_resp_msg amsg; >> + >> + /* >> + * Response message format: >> + * msg[0] is the return code of the operation >> + * msg[1] is the first parameter word >> + * msg[2] is the second parameter word >> + * >> + * As message only supports dword size, just assign it. >> + */ >> + >> + /* Check for sync query */ >> + if (ctx->resp_pending && >> + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || >> + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && >> + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || >> + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && >> + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && >> + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { >> + ctx->sync_msg.msg = ((u32 *) msg)[0]; >> + ctx->sync_msg.param1 = ((u32 *) msg)[1]; >> + ctx->sync_msg.param2 = ((u32 *) msg)[2]; >> + >> + /* Operation waiting for response */ >> + complete(&ctx->rd_complete); >> + >> + return; >> + } >> + >> + amsg.msg = ((u32 *) msg)[0]; >> + amsg.param1 = ((u32 *) msg)[1]; >> + amsg.param2 = ((u32 *) msg)[2]; >> + >> + /* Enqueue to the FIFO */ >> + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, >> + sizeof(struct slimpro_resp_msg), >> &ctx->kfifo_lock); >> + /* Schedule the bottom handler */ >> + schedule_work(&ctx->workq); >> +} >> + >> +/* >> + * This function is called when the PCC Mailbox received a message >> + */ >> +static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg) >> +{ >> + struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); >> + struct acpi_pcct_shared_memory *generic_comm_base = >> ctx->pcc_comm_addr; >> + struct slimpro_resp_msg amsg; >> + >> + msg = generic_comm_base + 1; >> + /* Check if platform sends interrupt */ >> + if (!xgene_word_tst_and_clr(&generic_comm_base->status, >> + PCCS_SCI_DOORBEL)) >> + return; >> + >> + /* >> + * Response message format: >> + * msg[0] is the return code of the operation >> + * msg[1] is the first parameter word >> + * msg[2] is the second parameter word >> + * >> + * As message only supports dword size, just assign it. >> + */ >> + >> + /* Check for sync query */ >> + if (ctx->resp_pending && >> + ((MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_ERR) || >> + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_DBG && >> + MSG_SUBTYPE(((u32 *) msg)[0]) == DBG_SUBTYPE_SENSOR_READ) || >> + (MSG_TYPE(((u32 *) msg)[0]) == MSG_TYPE_PWRMGMT && >> + MSG_SUBTYPE(((u32 *) msg)[0]) == PWRMGMT_SUBTYPE_TPC && >> + TPC_CMD(((u32 *) msg)[0]) == TPC_ALARM))) { >> + >> + /* Check if platform completes command */ >> + if (xgene_word_tst_and_clr(&generic_comm_base->status, >> + PCCS_CMD_COMPLETE)) { >> + ctx->sync_msg.msg = ((u32 *) msg)[0]; >> + ctx->sync_msg.param1 = ((u32 *) msg)[1]; >> + ctx->sync_msg.param2 = ((u32 *) msg)[2]; >> + >> + /* Operation waiting for response */ >> + complete(&ctx->rd_complete); >> + >> + return; >> + } >> + } >> + >> + /* >> + * Platform notifies interrupt to OSPM. >> + * OPSM schedules a consumer command to get this information >> + * in a workqueue. Platform must wait until OSPM has issued >> + * a consumer command that serves this notification. >> + */ >> + >> + /* Enqueue to the FIFO */ >> + kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg, >> + sizeof(struct slimpro_resp_msg), >> &ctx->kfifo_lock); >> + /* Schedule the bottom handler */ >> + schedule_work(&ctx->workq); >> +} >> + >> +static void xgene_hwmon_tx_done(struct mbox_client *cl, void *msg, int >> ret) >> +{ >> + if (ret) { >> + dev_dbg(cl->dev, "TX did not complete: CMD sent:%x, >> ret:%d\n", >> + *(u16 *) msg, ret); >> + } else { >> + dev_dbg(cl->dev, "TX completed. CMD sent:%x, ret:%d\n", >> + *(u16 *) msg, ret); > > > Please run your patch through checkoatch --strict. I would like to see > at least the following messages resolved. > > "No space is necessary after a cast" > "Blank lines aren't necessary after an open brace '{'" > "Please don't use multiple blank lines" > "Alignment should match open parenthesis" > "line over 80 characters" (with precedence over continuation line alignment) > > >> + } >> +} >> + >> +static int xgene_hwmon_probe(struct platform_device *pdev) >> +{ >> + struct xgene_hwmon_dev *ctx; >> + struct mbox_client *cl; >> + int rc; >> + >> + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); >> + if (!ctx) >> + return -ENOMEM; >> + >> + ctx->dev = &pdev->dev; >> + platform_set_drvdata(pdev, ctx); >> + cl = &ctx->mbox_client; >> + >> + /* Request mailbox channel */ >> + cl->dev = &pdev->dev; >> + cl->tx_done = xgene_hwmon_tx_done; >> + cl->tx_block = false; >> + cl->tx_tout = MBOX_OP_TIMEOUTMS; >> + cl->knows_txdone = false; >> + if (acpi_disabled) { >> + cl->rx_callback = xgene_hwmon_rx_cb; >> + ctx->mbox_chan = mbox_request_channel(cl, 0); >> + if (IS_ERR(ctx->mbox_chan)) { >> + dev_err(&pdev->dev, >> + "SLIMpro mailbox channel request >> failed\n"); >> + return -ENODEV; >> + } >> + } else { >> + struct acpi_pcct_hw_reduced *cppc_ss; >> + >> + if (device_property_read_u32(&pdev->dev, "pcc-channel", >> + &ctx->mbox_idx)) { >> + dev_err(&pdev->dev, "no pcc-channel property\n"); >> + return -ENODEV; >> + } >> + >> + cl->rx_callback = xgene_hwmon_pcc_rx_cb; >> + ctx->mbox_chan = pcc_mbox_request_channel(cl, >> ctx->mbox_idx); >> + if (IS_ERR(ctx->mbox_chan)) { >> + dev_err(&pdev->dev, >> + "PPC channel request failed\n"); >> + return -ENODEV; >> + } >> + >> + /* >> + * The PCC mailbox controller driver should >> + * have parsed the PCCT (global table of all >> + * PCC channels) and stored pointers to the >> + * subspace communication region in con_priv. >> + */ >> + cppc_ss = ctx->mbox_chan->con_priv; >> + if (!cppc_ss) { >> + dev_err(&pdev->dev, "PPC subspace not found\n"); >> + rc = -ENODEV; >> + goto out_mbox_free; >> + } >> + >> + if (!ctx->mbox_chan->mbox->txdone_irq) { >> + dev_err(&pdev->dev, "PCC IRQ not supported\n"); >> + rc = -ENODEV; >> + goto out_mbox_free; >> + } >> + >> + /* >> + * This is the shared communication region >> + * for the OS and Platform to communicate over. >> + */ >> + ctx->comm_base_addr = cppc_ss->base_address; >> + if (ctx->comm_base_addr) { >> + ctx->pcc_comm_addr = >> + >> acpi_os_ioremap(ctx->comm_base_addr, >> + cppc_ss->length); >> + } else { >> + dev_err(&pdev->dev, "Failed to get PCC comm >> region\n"); >> + rc = -ENODEV; >> + goto out_mbox_free; >> + } >> + >> + if (!ctx->pcc_comm_addr) { >> + dev_err(&pdev->dev, >> + "Failed to ioremap PCC comm region\n"); >> + rc = -ENOMEM; >> + goto out_mbox_free; >> + } >> + >> + /* >> + * cppc_ss->latency is just a Nominal value. In reality >> + * the remote processor could be much slower to reply. >> + * So add an arbitrary amount of wait on top of Nominal. >> + */ >> + ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency; >> + } >> + >> + spin_lock_init(&ctx->kfifo_lock); >> + mutex_init(&ctx->rd_mutex); >> + >> + rc = kfifo_alloc(&ctx->async_msg_fifo, >> + sizeof(struct slimpro_resp_msg) * >> ASYNC_MSG_FIFO_SIZE, >> + GFP_KERNEL); >> + if (rc) >> + goto out_mbox_free; >> + >> + INIT_WORK(&ctx->workq, xgene_hwmon_evt_work); >> + >> + ctx->hwmon_dev = devm_hwmon_device_register_with_groups(ctx->dev, >> + >> "apm_xgene", >> + ctx, >> + >> xgene_hwmon_groups); > > > Using the devm_ function here would, on remove, result in the mailbox and > kfifo > being removed before the hwmon driver is removed. This might result in race > conditions. You have two options: Use devm_add_action() to remove those in > sequence, > or use hwmon_device_register_with_groups() and remove the hwmon device > explicitly > (and first) in the remove function. Yes, I'll use hwmon_device_register_with_groups(). Thanks Hoan > > Note that you don't have to store hwmon_dev in ctx if you use > devm_add_action() > and devm_hwmon_device_register_with_groups(). > > >> + if (IS_ERR(ctx->hwmon_dev)) { >> + dev_err(&pdev->dev, "Failed to register HW monitor >> device\n"); >> + rc = PTR_ERR(ctx->hwmon_dev); >> + goto out; >> + } >> + >> + dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver >> registered\n"); >> + >> + return rc; >> + >> +out: >> + kfifo_free(&ctx->async_msg_fifo); >> +out_mbox_free: >> + if (acpi_disabled) >> + mbox_free_channel(ctx->mbox_chan); >> + else >> + pcc_mbox_free_channel(ctx->mbox_chan); >> + >> + return rc; >> +} >> + >> +static int xgene_hwmon_remove(struct platform_device *pdev) >> +{ >> + struct xgene_hwmon_dev *ctx = platform_get_drvdata(pdev); >> + >> + kfifo_free(&ctx->async_msg_fifo); >> + if (acpi_disabled) >> + mbox_free_channel(ctx->mbox_chan); >> + else >> + pcc_mbox_free_channel(ctx->mbox_chan); >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_ACPI >> +static const struct acpi_device_id xgene_hwmon_acpi_match[] = { >> + {"APMC0D29", 0}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(acpi, xgene_hwmon_acpi_match); >> +#endif >> + >> +static const struct of_device_id xgene_hwmon_of_match[] = { >> + {.compatible = "apm,xgene-slimpro-hwmon"}, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, xgene_hwmon_of_match); >> + >> +static struct platform_driver xgene_hwmon_driver __refdata = { >> + .probe = xgene_hwmon_probe, >> + .remove = xgene_hwmon_remove, >> + .driver = { >> + .name = "xgene-slimpro-hwmon", >> + .of_match_table = xgene_hwmon_of_match, >> + .acpi_match_table = ACPI_PTR(xgene_hwmon_acpi_match), >> + }, >> +}; >> +module_platform_driver(xgene_hwmon_driver); >> + >> +MODULE_DESCRIPTION("APM X-Gene SoC hardware monitor"); >> +MODULE_LICENSE("GPL"); >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] arm64: dts: apm: Add X-Gene SoC hwmon to device tree 2016-07-12 0:30 [PATCH v2 0/3] hwmon: xgene: Add support for X-Gene hwmon driver Hoan Tran 2016-07-12 0:30 ` [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation Hoan Tran 2016-07-12 0:30 ` [PATCH v2 2/3] hwmon: xgene: Add hwmon driver Hoan Tran @ 2016-07-12 0:30 ` Hoan Tran 2 siblings, 0 replies; 7+ messages in thread From: Hoan Tran @ 2016-07-12 0:30 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, Jonathan Corbet, Rob Herring Cc: Jassi Brar, Ashwin Chaugule, linux-hwmon, linux-doc, linux-kernel, linux-arm-kernel, devicetree, Duc Dang, lho, Hoan Tran This patch adds DT node to enable hwmon driver for APM X-Gene SoC. Signed-off-by: Hoan Tran <hotran@apm.com> --- arch/arm64/boot/dts/apm/apm-shadowcat.dtsi | 5 +++++ arch/arm64/boot/dts/apm/apm-storm.dtsi | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi b/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi index c569f76..a5935f5 100644 --- a/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi +++ b/arch/arm64/boot/dts/apm/apm-shadowcat.dtsi @@ -472,6 +472,11 @@ mboxes = <&mailbox 0>; }; + hwmonslimpro { + compatible = "apm,xgene-slimpro-hwmon"; + mboxes = <&mailbox 7>; + }; + serial0: serial@10600000 { device_type = "serial"; compatible = "ns16550"; diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi index 5147d76..f8ea5b5 100644 --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi @@ -716,6 +716,11 @@ mboxes = <&mailbox 0>; }; + hwmonslimpro { + compatible = "apm,xgene-slimpro-hwmon"; + mboxes = <&mailbox 7>; + }; + serial0: serial@1c020000 { status = "disabled"; device_type = "serial"; -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-18 21:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-12 0:30 [PATCH v2 0/3] hwmon: xgene: Add support for X-Gene hwmon driver Hoan Tran 2016-07-12 0:30 ` [PATCH v2 1/3] Documentation: dtb: xgene: Add hwmon dts binding documentation Hoan Tran 2016-07-16 19:28 ` Rob Herring 2016-07-12 0:30 ` [PATCH v2 2/3] hwmon: xgene: Add hwmon driver Hoan Tran 2016-07-16 16:35 ` Guenter Roeck 2016-07-18 21:30 ` Hoan Tran 2016-07-12 0:30 ` [PATCH v2 3/3] arm64: dts: apm: Add X-Gene SoC hwmon to device tree Hoan Tran
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).