Re: [2/3] hwmon: xgene: Adds hwmon driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Guenter,

Thanks for your review !

On Sun, May 29, 2016 at 10:25 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On Mon, May 16, 2016 at 09:17:26AM -0700, hotran wrote:
>> This patch adds hardware temperature and power reading support for
>> APM X-Gene SoC's using the mailbox communication interface.
>>
> Please drop the "'".

Yes, will remove it.

>
>>
>> Signed-off-by: Hoan Tran <hotran@xxxxxxx>
>> ---
>>  Documentation/hwmon/xgene-hwmon |  32 ++
>>  drivers/hwmon/Kconfig           |   7 +
>>  drivers/hwmon/Makefile          |   1 +
>>  drivers/hwmon/xgene-hwmon.c     | 801 ++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 841 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..040a7f2
>> --- /dev/null
>> +++ b/Documentation/hwmon/xgene-hwmon
>> @@ -0,0 +1,32 @@
>> +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's using the mailbox communication interface.
>>
> No "'".

Yes,

>>
>> +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
>> +    - Alarm when high/over temperature occurs
>> +  * Power
>> +    - CPU power in uW
>> +    - IO power in uW
>> +    - CPU and IO power in uW
>> +
>> +sysfs-Interface
>> +---------------
>> +
>> +temp1_input - SoC on-die temperature (milli-degree C)
>> +temp1_critical_alarm - An 1 would indicates on-die temperature exceeded threshold
>> +power1_input - CPU power in (uW)
>> +power2_input - IO power in (uW)
>> +power3_input - CPU and IO power in (uW)
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 5c2d13a..91c3056 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1776,6 +1776,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 58cc3ac..668d0f1 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -161,6 +161,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..bc4ad29
>> --- /dev/null
>> +++ b/drivers/hwmon/xgene-hwmon.c
>> @@ -0,0 +1,801 @@
>> +/*
>> + * APM X-Gene SoC Hardware Monitoring Driver
>> + *
>> + * Copyright (c) 2016, Applied Micro Circuits Corporation
>> + * Author: Loc Ho <lho@xxxxxxx>
>> + *         Hoan Tran <hotran@xxxxxxx>
>> + *
>> + * 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's total power (uW)
>> + *  - Retrieve IO's total power (uW)
>
> Please drop the "'s".

Yes,

>
>> + *  - Retrieve SoC 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_client.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <acpi/cppc_acpi.h>
>> +
>> +/* SLIMpro message defines */
>> +#define SLIMPRO_MSG_TYPE_DBG_ID              0
>> +#define SLIMPRO_MSG_TYPE_ERR_ID              7
>> +#define SLIMPRO_MSG_TYPE_PWRMGMT_ID  9
>> +
>> +#define SLIMPRO_MSG_TYPE(v)          (((v) & 0xF0000000) >> 28)
>> +#define SLIMPRO_MSG_TYPE_SET(v)              (((v) << 28) & 0xF0000000)
>> +#define SLIMPRO_MSG_SUBTYPE(v)               (((v) & 0x0F000000) >> 24)
>> +#define SLIMPRO_MSG_SUBTYPE_SET(v)   (((v) << 24) & 0x0F000000)
>> +
>> +#define SLIMPRO_DBG_SUBTYPE_SENSOR_READ      4
>> +#define SLIMPRO_SENSOR_READ_MSG              0x04FFE902
>> +#define SLIMPRO_SENSOR_READ_ENCODE_ADDR(a) \
>> +     ((a) & 0x000FFFFF)
>> +#define PMD_PWR_MW_REG                       0x26
>> +#define SOC_PWR_REG                  0x21
>> +#define SOC_TEMP_REG                 0x10
>> +
>> +#define SLIMPRO_PWRMGMT_SUBTYPE_TPC  1
>> +#define SLIMPRO_TPC_ALARM            2
>> +#define SLIMPRO_TPC_GET_ALARM                3
>> +#define SLIMPRO_TPC_CMD(v)           (((v) & 0x00FF0000) >> 16)
>> +#define SLIMPRO_TPC_CMD_SET(v)               (((v) << 16) & 0x00FF0000)
>> +#define SLIMPRO_TPC_ENCODE_MSG(hndl, cmd, type) \
>> +     (SLIMPRO_MSG_TYPE_SET(SLIMPRO_MSG_TYPE_PWRMGMT_ID) | \
>> +     SLIMPRO_MSG_SUBTYPE_SET(hndl) | \
>> +     SLIMPRO_TPC_CMD_SET(cmd) | \
>> +     type)
>> +
>
> Do you _have_ to use such long defines ? This causes lots of "line too long"
> checkpatch warnings, which would really be easy to avoid with shorter defines.

Ok, I'll shorten these defines.

>
>> +/* PCC defines */
>> +#define SLIMPRO_MSG_PCC_SUBSPACE     7
>> +#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_HWMON_INDEX             0
>> +#define MBOX_OP_TIMEOUTMS            1000
>> +
>> +#define SOC_TEMP                     0
>> +#define CPU_POWER                    0
>> +#define IO_POWER                     1
>> +#define SOC_POWER                    2
>> +
>
> Are those random defines, to be used for index values ?
> If so, how about enums ?

Yes, will move them into an enum.

>
>> +#define WATT_TO_mWATT(x)             ((x) * 1000)
>> +#define mWATT_TO_uWATT(x)            ((x) * 1000)
>> +#define WATT_TO_uWATT(x)             ((x) * 1000000)
>> +#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;
>> +
>> +     spinlock_t              lock;
>> +     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_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;
>> +     unsigned long flags;
>> +     u16 val;
>> +     int rc;
>> +
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +     if (ctx->resp_pending) {
>> +             spin_unlock_irqrestore(&ctx->lock, flags);
>> +             return -EAGAIN;
>> +     }
>> +
>> +     init_completion(&ctx->rd_complete);
>> +     ctx->resp_pending = true;
>> +
>> +     /* Write signature for subspace */
>> +     writel_relaxed(PCC_SIGNATURE_MASK | SLIMPRO_MSG_PCC_SUBSPACE,
>> +                    &generic_comm_base->signature);
>> +
>> +     /* Write to the shared command region */
>> +     writew_relaxed(SLIMPRO_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 */
>> +     memcpy(ptr, msg, sizeof(struct slimpro_resp_msg));
>> +
>> +     /* 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;
>> +     }
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +     if (!wait_for_completion_timeout(&ctx->rd_complete,
>> +                                      usecs_to_jiffies(ctx->usecs_lat))) {
>> +             spin_lock_irqsave(&ctx->lock, flags);
>> +             dev_err(ctx->dev, "Mailbox operation timed out\n");
>> +             rc = -EIO;
>
>         -ETIMEDOUT ?

OK

>
>> +             goto err;
>> +     }
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +
>> +     /* Check for invalid data or no device */
>> +     if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
>> +         ctx->sync_msg.msg == 0xffffffff) {
>> +             rc = -ENODEV;
>
> Two different errors ?

Yes, will fix it

>
>> +             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;
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +     return rc;
>> +}
>> +
>> +static int xgene_hwmon_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>> +{
>> +     unsigned long flags;
>> +     int rc;
>> +
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +     if (ctx->resp_pending) {
>> +             spin_unlock_irqrestore(&ctx->lock, flags);
>> +             return -EAGAIN;
>> +     }
>> +
>> +     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;
>> +     }
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +
>> +     if (!wait_for_completion_timeout(&ctx->rd_complete,
>> +                                      msecs_to_jiffies(MBOX_OP_TIMEOUTMS))) {
>> +             spin_lock_irqsave(&ctx->lock, flags);
>> +             dev_err(ctx->dev, "Mailbox operation timed out\n");
>> +             rc = -EIO;
>
>         -ETIMEDOUT ?

OK

>
>> +             goto err;
>> +     }
>> +     spin_lock_irqsave(&ctx->lock, flags);
>> +
>> +     /* Check for invalid data or no device */
>> +     if (SLIMPRO_MSG_TYPE(ctx->sync_msg.msg) == SLIMPRO_MSG_TYPE_ERR_ID ||
>> +         ctx->sync_msg.msg == 0xffffffff) {
>> +             rc = -ENODEV;
>
> Isn't that two different errors ?

Will fix it.

>
>> +             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;
>> +     spin_unlock_irqrestore(&ctx->lock, flags);
>> +     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] = SLIMPRO_SENSOR_READ_MSG;
>> +     msg[1] = SLIMPRO_SENSOR_READ_ENCODE_ADDR(addr);
>> +     msg[2] = 0;
>> +
> Does the msg endianness always match the CPU endianness ?
> Probably yes, just want to make sure.

For DT, SLIMpro mailbox controller will take care of endianness.
For ACPI, maybe I missed it, it should be corrected when PCC writes
message into shared memory. Thanks

>
>> +     if (ACPI_COMPANION(ctx->dev))
>> +             rc = xgene_hwmon_pcc_rd(ctx, msg);
>> +     else
>> +             rc = xgene_hwmon_rd(ctx, msg);
>> +     if (rc < 0) {
>> +             /* To support compatibility with firmware, return 0 */
>> +             dev_err(ctx->dev, "SLIMpro register 0x%02X read error %d\n",
>> +                     addr, rc);
>
> Is this error mesage necessary ?

Will remove it.

>
>> +             *data = 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] = SLIMPRO_TPC_ENCODE_MSG(SLIMPRO_PWRMGMT_SUBTYPE_TPC,
>> +                                     SLIMPRO_TPC_GET_ALARM, 0);
>> +     msg[1] = 0;
>> +     msg[2] = 0;
>> +
>> +     rc = xgene_hwmon_pcc_rd(ctx, msg);
>> +     if (rc < 0) {
>> +             dev_err(ctx->dev, "PCC Alarm read error %d\n", rc);
>
> Is this error message necessary ?

Will remove it.

>
>> +             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)
>> +{
>> +     return xgene_hwmon_reg_map_rd(ctx, PMD_PWR_MW_REG, val);
>> +}
>> +
>> +static int xgene_hwmon_get_io_pwr(struct xgene_hwmon_dev *ctx, u32 *val)
>> +{
>> +     return xgene_hwmon_reg_map_rd(ctx, SOC_PWR_REG, val);
>> +}
>> +
>> +static int xgene_hwmon_get_soc_power(struct xgene_hwmon_dev *ctx, u32 *val)
>> +{
>> +     u32 pmd_vrm_power;
>> +     u32 io_vrm_power;
>> +     int rc;
>> +
>> +     rc = xgene_hwmon_get_cpu_pwr(ctx, &pmd_vrm_power);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     rc = xgene_hwmon_get_io_pwr(ctx, &io_vrm_power);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     *val = pmd_vrm_power + WATT_TO_mWATT(io_vrm_power);
>> +
>
> So the SoC power is just adding CPU and IO power, ie it reports the sum
> of both ?  Please drop. Such calculations should be done in user space
> if wanted.

Ok, will drop this SoC power.

>
>> +     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 const char * const sensor_temp_input_names[] = {
>> +     [SOC_TEMP] = "SoC Temperature",
>> +};
>> +
>> +static const char * const sensor_pwr_input_names[] = {
>> +     [CPU_POWER] = "CPU's power",
>> +     [IO_POWER] = "IO's power",
>
> Please drop the "'s".

Ok,

>
>> +     [SOC_POWER] = "SoC power"
>> +};
>> +
>> +static ssize_t xgene_hwmon_show_name(struct device *dev,
>> +                                  struct device_attribute *attr, char *buf)
>> +{
>> +     return snprintf(buf, PAGE_SIZE, "APM X-Gene\n");
>> +}
>> +
>> +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);
>> +     u32 val;
>> +     int rc;
>> +
>> +     rc = xgene_hwmon_get_temp(ctx, &val);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     val = CELSIUS_TO_mCELSIUS(val);
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", val);
>
> val is u32 (is it reported by the chip as unsigned ?)

It also give a negative temp. I'll fix it

>
>> +}
>> +
>> +static ssize_t xgene_hwmon_show_temp_label(struct device *dev,
>> +                                        struct device_attribute *attr,
>> +                                        char *buf)
>> +{
>> +     int channel = to_sensor_dev_attr(attr)->index - 1;
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%s\n",
>> +                     sensor_temp_input_names[channel]);
>> +}
>> +
>> +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_pwr_label(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       char *buf)
>> +{
>> +     int channel = to_sensor_dev_attr(attr)->index - 1;
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%s\n",
>> +                     sensor_pwr_input_names[channel]);
>> +}
>> +
>> +static ssize_t xgene_hwmon_show_pwr(struct device *dev,
>> +                                 struct device_attribute *attr,
>> +                                 char *buf)
>> +{
>> +     struct xgene_hwmon_dev *ctx = dev_get_drvdata(dev);
>> +     int channel = to_sensor_dev_attr(attr)->index - 1;
>> +     u32 val;
>> +     int rc;
>> +
>> +     switch (channel) {
>> +     case CPU_POWER:
>> +             rc = xgene_hwmon_get_cpu_pwr(ctx, &val);
>> +             break;
>> +     case IO_POWER:
>> +             rc = xgene_hwmon_get_io_pwr(ctx, &val);
>> +             break;
>> +     case SOC_POWER:
>> +             rc = xgene_hwmon_get_soc_power(ctx, &val);
>> +             break;
>
> Those are not the index values specified. Please use those constants
> in the attribute declaration.
>
> Overall, it would probably be easier to just have two separate
> functions for CPU an IO power.

Yes, I'll use separate functions.

>
>> +     default:
>> +             rc = -EINVAL;
>> +             break;
>> +     }
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     if (channel == IO_POWER)
>> +             val = WATT_TO_uWATT(val);
>
> What is the maximum possible reported IO power ? I understand that
> the maximum supported value with the current code is 4,294 kW, but
> if the hardware can report more than that it would be prudent to
> use an unsigned long for the value.

This is the power of internal SoC and it could not exceed this number

>
>> +     else
>> +             val = mWATT_TO_uWATT(val);
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", val);
>
> val is u32.

Yes, change to %u.

>
>> +}
>> +
>> +/* Chip name, required by hwmon */
>> +static DEVICE_ATTR(name, S_IRUGO, xgene_hwmon_show_name, NULL);
>> +
>
> Unnecesary - see below.
>
>> +#define SENSOR_ATTR_TEMP(index) \
>> +     static SENSOR_DEVICE_ATTR(temp##index##_label, S_IRUGO, \
>> +                               xgene_hwmon_show_temp_label, NULL, index); \
>> +     static SENSOR_DEVICE_ATTR(temp##index##_input, S_IRUGO, \
>> +                               xgene_hwmon_show_temp, NULL, index); \
>> +     static SENSOR_DEVICE_ATTR(temp##index##_critical_alarm, S_IRUGO, \
>> +                               xgene_hwmon_show_temp_critical_alarm, NULL, \
>> +                               index);
>> +#define SENSOR_ATTR_PWR(index) \
>> +     static SENSOR_DEVICE_ATTR(power##index##_label, S_IRUGO, \
>> +                               xgene_hwmon_show_pwr_label, NULL, index); \
>> +     static SENSOR_DEVICE_ATTR(power##index##_input, S_IRUGO, \
>> +                               xgene_hwmon_show_pwr, NULL, index);
>> +
>> +#define APM_TEMP_SENSOR_ATTR(index) \
>> +     &sensor_dev_attr_temp##index##_input.dev_attr.attr, \
>> +     &sensor_dev_attr_temp##index##_label.dev_attr.attr, \
>> +     &sensor_dev_attr_temp##index##_critical_alarm.dev_attr.attr
>> +#define APM_PWR_SENSOR_ATTR(index) \
>> +     &sensor_dev_attr_power##index##_input.dev_attr.attr, \
>> +     &sensor_dev_attr_power##index##_label.dev_attr.attr
>> +
>> +SENSOR_ATTR_TEMP(1); /* SoC temperature */
>> +SENSOR_ATTR_PWR(1);  /* CPU power */
>> +SENSOR_ATTR_PWR(2);  /* IO power */
>> +SENSOR_ATTR_PWR(3);  /* SoC power */
>> +
>> +static struct attribute *xgene_hwmon_attributes[] = {
>> +     &dev_attr_name.attr,
>> +     APM_TEMP_SENSOR_ATTR(1),
>> +     APM_PWR_SENSOR_ATTR(1),
>> +     APM_PWR_SENSOR_ATTR(2),
>> +     APM_PWR_SENSOR_ATTR(3),
>
> Please drop the macros and write explicit code.
>
> 'index' values are always accessed with '- 1'. Please start the index with 0
> and drop the '- 1'.

Ok, change to an explicit code.

>
>> +     NULL,
>> +};
>> +
>> +static const struct attribute_group xgene_hwmon_attr_group = {
>> +     .attrs  = xgene_hwmon_attributes,
>> +};
>> +
>> +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), "temp%d_critical_alarm", SOC_TEMP + 1);
>> +     sysfs_notify(&ctx->dev->kobj, NULL, name);
>> +     dev_alert(ctx->dev, "SoC temperature alarm at %d degree\n",
>> +               amsg->param1);
>
> Please drop this message. That is what the notification is for.
> User space can then decide if it wants to log or not.
>

Ok

>> +     return 0;
>> +}
>> +
>> +static void xgene_hwmon_process_pwrmsg(struct xgene_hwmon_dev *ctx,
>> +                                    struct slimpro_resp_msg *amsg)
>> +{
>> +     switch (SLIMPRO_MSG_SUBTYPE(amsg->msg)) {
>> +     case SLIMPRO_PWRMGMT_SUBTYPE_TPC:
>> +             switch (SLIMPRO_TPC_CMD(amsg->msg)) {
>> +             case SLIMPRO_TPC_ALARM:
>> +                     xgene_hwmon_tpc_alarm(ctx, amsg);
>> +                     break;
>> +             default:
>> +                     dev_warn(ctx->dev,
>> +                              "Un-supported TPC message received 0x%08X\n",
>> +                              amsg->msg);
>> +                     break;
>> +             }
>> +             break;
>> +     default:
>> +             dev_warn(ctx->dev, "Un-supported message received 0x%08X\n",
>> +                      amsg->msg);
>
> Are those messages necessary ? Can they clog the log ?

Will remove it.

>
>> +             break;
>> +     }
>> +}
>> +
>> +/*
>> + * 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->lock)) {
>> +             /*
>> +              * If PCC, send a consumer command to Platform to get info
>> +              * If Slimpro Mailbox, get message from specific FIFO
>> +              */
>> +             if (ACPI_COMPANION(ctx->dev)) {
>> +                     ret = xgene_hwmon_get_notification_msg(ctx,
>> +                                                            (u32 *)&amsg);
>> +                     if (ret < 0)
>> +                             continue;
>> +             }
>> +             switch (SLIMPRO_MSG_TYPE(amsg.msg)) {
>> +             case SLIMPRO_MSG_TYPE_PWRMGMT_ID:
>> +                     xgene_hwmon_process_pwrmsg(ctx, &amsg);
>> +                     break;
>> +             default:
>> +                     dev_warn(ctx->dev,
>> +                              "Invalid mailbox msg received 0x%08X 0x%08X 0x%08X\n",
>> +                              amsg.msg, amsg.param1, amsg.param2);
>
> Is this message necessary ? I am concerned about the logging noise it might
> create.

Will remove it.

>
>> +                     break;
>> +             }
>> +     }
>> +}
>> +
>> +/*
>> + * This function is called when the SLIMpro/PCC 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 acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
>> +     struct slimpro_resp_msg amsg;
>> +
>> +     /* If PCC mailbox controller, get msg from shared memory */
>> +     if (ACPI_COMPANION(ctx->dev)) {
>> +             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 &&
>> +         ((SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_ERR_ID) ||
>> +          (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_DBG_ID &&
>> +           SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_DBG_SUBTYPE_SENSOR_READ) ||
>> +          (SLIMPRO_MSG_TYPE(((u32 *) msg)[0]) == SLIMPRO_MSG_TYPE_PWRMGMT_ID &&
>> +           SLIMPRO_MSG_SUBTYPE(((u32 *) msg)[0]) == SLIMPRO_PWRMGMT_SUBTYPE_TPC &&
>> +           SLIMPRO_TPC_CMD(((u32 *) msg)[0]) == SLIMPRO_TPC_ALARM))) {
>> +             if (ACPI_COMPANION(ctx->dev)) {
>> +                     /* Check if platform completes command */
>> +                     if (!xgene_word_tst_and_clr(&generic_comm_base->status,
>> +                                                 PCCS_CMD_COMPLETE))
>> +                             goto notify;
>
> How about reversing the logic here and dropping the goto ?

OK,

>
> Overall, I have to say that the code is quite complex due to the repeated
> checks for ACPI. I am close to suggest having two separate drivers,
> one for ACPI and one for non-ACPI. Any chance to separate ACPI vs. non-ACPI
> code a bit better ?

How about create separate rx_cb functions for ACPI and non-ACPI ?
As almost functions are the same between ACPI and non-ACPI, I thought
keep the same driver is still OK, isn't it ?

>
>> +             }
>> +             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;
>> +     }
>> +
>> +     /*
>> +      * If PCC, 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.
>> +      */
>> +notify:
>> +     if (ACPI_COMPANION(ctx->dev)) {
>> +             /* Check and clear Platform Notification bit */
>> +             if (!xgene_word_tst_and_clr(&generic_comm_base->status,
>> +                                         PCCS_PLATFORM_NOTIFICATION))
>> +                     return;
>> +     } else {
>> +             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->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 __init 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->rx_callback = xgene_hwmon_rx_cb;
>> +     cl->tx_done = xgene_hwmon_tx_done;
>> +     cl->tx_block = false;
>> +     cl->tx_tout = MBOX_OP_TIMEOUTMS;
>> +     cl->knows_txdone = false;
>> +     if (!ACPI_COMPANION(cl->dev)) {
>> +             ctx->mbox_chan = mbox_request_channel(cl, MBOX_HWMON_INDEX);
>> +             if (IS_ERR(ctx->mbox_chan)) {
>> +                     dev_err(&pdev->dev,
>> +                             "SLIMpro mailbox channel request failed\n");
>> +                     return PTR_ERR(ctx->mbox_chan);
>> +             }
>> +     } else {
>> +             struct acpi_pcct_hw_reduced *cppc_ss;
>> +
>> +             ctx->mbox_chan =
>> +                     pcc_mbox_request_channel(cl, SLIMPRO_MSG_PCC_SUBSPACE);
>> +             if (IS_ERR(ctx->mbox_chan)) {
>> +                     dev_err(&pdev->dev,
>> +                             "PPC channel request failed\n");
>> +                     return PTR_ERR(ctx->mbox_chan);
>> +             }
>> +
>> +             /*
>> +              * 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->lock);
>> +
>> +     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);
>> +
>> +     /* Hook up sysfs for sensor monitor */
>> +     rc = sysfs_create_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
>> +     if (rc) {
>> +             dev_err(&pdev->dev, "Fail to create sysfs\n");
>> +             goto out_kfifo_free;
>> +     }
>> +
>> +     ctx->hwmon_dev = hwmon_device_register(ctx->dev);
>> +     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;
>> +     }
>
> Please use hwmon_device_register_with_groups().

OK,

Thanks and Regards
Hoan
>
>> +
>> +     dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver registered\n");
>> +
>> +     return rc;
>> +
>> +out:
>> +     sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
>> +out_kfifo_free:
>> +     kfifo_free(&ctx->async_msg_fifo);
>> +out_mbox_free:
>> +     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);
>> +
>> +     hwmon_device_unregister(ctx->hwmon_dev);
>> +     sysfs_remove_group(&pdev->dev.kobj, &xgene_hwmon_attr_group);
>> +     kfifo_free(&ctx->async_msg_fifo);
>> +     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");
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux