* [PATCH v2 0/2] hwmon: (pmbus) add debugfs features for Gen 2 Renesas digital multiphase
@ 2020-04-22 17:26 Grant Peltier
2020-04-22 17:27 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints Grant Peltier
2020-04-22 17:27 ` [PATCH v2 2/2] docs: hwmon: Update driver documentation for debugfs Grant Peltier
0 siblings, 2 replies; 6+ messages in thread
From: Grant Peltier @ 2020-04-22 17:26 UTC (permalink / raw)
To: linux, linux-hwmon, linux-kernel; +Cc: adam.vaughn.xh, grant.peltier.jg
The patch series adds debugfs features for 2nd generation Renesas digital
multiphase voltage regulators. The new debugfs endpoints allow users to
utilize features of the VR that are not compatible with sysfs.
The series contains 2 patches:
- patch #1 adds the new debugfs features to the isl68137 driver
- patch #2 adds documentation for the new debugfs endpoints
Grant Peltier (2):
hwmon: (pmbus/isl68137) add debugfs config and black box endpoints
docs: hwmon: Update driver documentation for debugfs
Documentation/hwmon/isl68137.rst | 34 ++-
drivers/hwmon/pmbus/isl68137.c | 460 ++++++++++++++++++++++++++++++-
2 files changed, 490 insertions(+), 4 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints
2020-04-22 17:26 [PATCH v2 0/2] hwmon: (pmbus) add debugfs features for Gen 2 Renesas digital multiphase Grant Peltier
@ 2020-04-22 17:27 ` Grant Peltier
2020-04-25 17:43 ` Guenter Roeck
2020-04-22 17:27 ` [PATCH v2 2/2] docs: hwmon: Update driver documentation for debugfs Grant Peltier
1 sibling, 1 reply; 6+ messages in thread
From: Grant Peltier @ 2020-04-22 17:27 UTC (permalink / raw)
To: linux, linux-hwmon, linux-kernel; +Cc: adam.vaughn.xh, grant.peltier.jg
Add debugfs endpoints to support features of 2nd generation Renesas
digital multiphase voltage regulators that are not compatible with
sysfs.
The read_black_box endpoint allows users to read the contents of a
RAM segment used to record fault conditions within Gen 2 devices.
The write_config endpoint allows users to write configuration hex
files to Gen 2 devices which modify how they operate.
Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
drivers/hwmon/pmbus/isl68137.c | 460 ++++++++++++++++++++++++++++++++-
1 file changed, 457 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
index 0c622711ef7e..455235a7a56b 100644
--- a/drivers/hwmon/pmbus/isl68137.c
+++ b/drivers/hwmon/pmbus/isl68137.c
@@ -7,10 +7,12 @@
*
*/
+#include <linux/debugfs.h>
#include <linux/err.h>
#include <linux/hwmon-sysfs.h>
#include <linux/i2c.h>
#include <linux/init.h>
+#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/string.h>
@@ -18,8 +20,22 @@
#include "pmbus.h"
-#define ISL68137_VOUT_AVS 0x30
-#define RAA_DMPVR2_READ_VMON 0xc8
+#define ISL68137_VOUT_AVS 0x30
+#define RAA_DMPVR2_DMA_FIX 0xc5
+#define RAA_DMPVR2_DMA_SEQ 0xc6
+#define RAA_DMPVR2_DMA_ADDR 0xc7
+#define RAA_DMPVR2_READ_VMON 0xc8
+#define RAA_DMPVR2_BB_BASE_ADDR 0xef80
+#define RAA_DMPVR2_BB_WSIZE 4
+#define RAA_DMPVR2_BB_WCNT 32
+#define RAA_DMPVR2_BB_BUF_SIZE 288
+#define RAA_DMPVR2_NVM_CNT_ADDR 0x00c2
+#define RAA_DMPVR2_PRGM_STATUS_ADDR 0x0707
+#define RAA_DMPVR2_BANK0_STATUS_ADDR 0x0709
+#define RAA_DMPVR2_BANK1_STATUS_ADDR 0x070a
+#define RAA_DMPVR2_CFG_MAX_SLOT 16
+#define RAA_DMPVR2_CFG_HEAD_LEN 290
+#define RAA_DMPVR2_CFG_SLOT_LEN 358
enum chips {
isl68137,
@@ -71,6 +87,21 @@ enum variants {
raa_dmpvr2_hv,
};
+enum {
+ RAA_DMPVR2_DEBUGFS_CFG_W = 0,
+ RAA_DMPVR2_DEBUGFS_BB_R,
+ RAA_DMPVR2_DEBUGFS_NUM_ENTRIES,
+};
+
+struct raa_dmpvr2_ctrl {
+ enum chips part;
+ struct i2c_client *client;
+ int debugfs_entries[RAA_DMPVR2_DEBUGFS_NUM_ENTRIES];
+};
+
+#define to_ctrl(x, y) container_of((x), struct raa_dmpvr2_ctrl, \
+ debugfs_entries[(y)])
+
static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
int page,
char *buf)
@@ -157,6 +188,395 @@ static const struct attribute_group *isl68137_attribute_groups[] = {
NULL,
};
+/**
+ * Helper function required since linux SMBus implementation does not currently
+ * (v5.6) support the SMBus 3.0 "Read 32" protocol
+ */
+static s32 raa_dmpvr2_smbus_read32(const struct i2c_client *client, u8 command,
+ unsigned char *data)
+{
+ int status;
+ unsigned char msgbuf[1];
+ struct i2c_msg msg[2] = {
+ {
+ .addr = client->addr,
+ .flags = client->flags,
+ .len = 1,
+ .buf = msgbuf,
+ },
+ {
+ .addr = client->addr,
+ .flags = client->flags | I2C_M_RD,
+ .len = 4,
+ .buf = data,
+ },
+ };
+
+ msgbuf[0] = command;
+ status = i2c_transfer(client->adapter, msg, 2);
+ if (status != 2)
+ return status;
+ return 0;
+}
+
+/**
+ * Helper function required since linux SMBus implementation does not currently
+ * (v5.6) support the SMBus 3.0 "Write 32" protocol
+ */
+static s32 raa_dmpvr2_smbus_write32(const struct i2c_client *client,
+ u8 command, u32 value)
+{
+ int status;
+ unsigned char msgbuf[5];
+ struct i2c_msg msg[1] = {
+ {
+ .addr = client->addr,
+ .flags = client->flags,
+ .len = 5,
+ .buf = msgbuf,
+ },
+ };
+ msgbuf[0] = command;
+ msgbuf[1] = value & 0x0FF;
+ msgbuf[2] = (value >> 8) & 0x0FF;
+ msgbuf[3] = (value >> 16) & 0x0FF;
+ msgbuf[4] = (value >> 24) & 0x0FF;
+ status = i2c_transfer(client->adapter, msg, 1);
+ if (status != 1)
+ return status;
+ return 0;
+}
+
+static ssize_t raa_dmpvr2_read_black_box(struct raa_dmpvr2_ctrl *ctrl,
+ char __user *buf, size_t len,
+ loff_t *ppos)
+{
+ int i, j;
+ u16 addr = RAA_DMPVR2_BB_BASE_ADDR;
+ unsigned char data[RAA_DMPVR2_BB_WSIZE] = { 0 };
+ char out[RAA_DMPVR2_BB_BUF_SIZE] = { 0 };
+ char *optr = out;
+
+ i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR, addr);
+ for (i = 0; i < RAA_DMPVR2_BB_WCNT; i++) {
+ raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ,
+ data);
+ for (j = 0; j < RAA_DMPVR2_BB_WSIZE; j++)
+ optr += snprintf(optr, sizeof(out), "%02X", data[j]);
+ *optr = '\n';
+ optr++;
+ }
+
+ return simple_read_from_buffer(buf, len, ppos, &out, sizeof(out));
+}
+
+struct raa_dmpvr_cfg_cmd {
+ u8 cmd;
+ u8 len;
+ u8 data[4];
+};
+
+struct raa_dmpvr2_cfg {
+ u8 dev_id[4];
+ u8 dev_rev[4];
+ int slot_cnt;
+ int cmd_cnt;
+ struct raa_dmpvr_cfg_cmd *cmds;
+ u8 crc[4];
+};
+
+/**
+ * Helper function to handle hex string to byte conversions
+ */
+static int raa_dmpvr2_hextou8(char *buf, u8 *res)
+{
+ char s[3];
+
+ s[0] = buf[0];
+ s[1] = buf[1];
+ s[2] = '\0';
+ return kstrtou8(s, 16, res);
+}
+
+static int raa_dmpvr2_parse_cfg(char *buf, struct raa_dmpvr2_cfg *cfg)
+{
+ const int lsta = 2;
+ const int csta = 6;
+ const int dsta = 8;
+ char *cptr, *line;
+ int lcnt = 1;
+ int ccnt = 0;
+ int i, j, ret;
+ u8 b;
+
+ // Ensure there is enough memory for the file
+ for (i = 0; i < strlen(buf); i++) {
+ if (buf[i] == '\n' && i < strlen(buf)-2) {
+ lcnt++;
+ if (buf[i+1] != '4' || buf[i+2] != '9')
+ ccnt++;
+ }
+ }
+ cfg->slot_cnt = (lcnt-RAA_DMPVR2_CFG_HEAD_LEN)/RAA_DMPVR2_CFG_SLOT_LEN;
+ if (cfg->slot_cnt < 1 || cfg->slot_cnt > RAA_DMPVR2_CFG_MAX_SLOT)
+ return -EINVAL;
+ cfg->cmd_cnt = ccnt;
+ cfg->cmds = kmalloc_array(ccnt, sizeof(struct raa_dmpvr_cfg_cmd),
+ GFP_KERNEL);
+ if (!cfg->cmds)
+ return -ENOMEM;
+
+ // Parse header
+ for (i = 0; i < 2; i++) {
+ line = strsep(&buf, "\n");
+ cptr = line + dsta;
+ for (j = 3; j >= 0; j--) {
+ ret = raa_dmpvr2_hextou8(cptr, &b);
+ if (ret)
+ goto parse_err;
+ if (i == 0)
+ cfg->dev_id[j] = b;
+ else
+ cfg->dev_rev[j] = b;
+ cptr += 2;
+ }
+ }
+
+ // Parse cmds
+ i = 0;
+ while (line != NULL && strlen(line) > (dsta + 2)) {
+ if (strncmp(line, "49", 2) != 0) {
+ ret = raa_dmpvr2_hextou8(line+lsta, &b);
+ if (ret)
+ goto parse_err;
+ cfg->cmds[i].len = b - 3;
+ ret = raa_dmpvr2_hextou8(line+csta, &b);
+ if (ret)
+ goto parse_err;
+ cfg->cmds[i].cmd = b;
+ for (j = 0; j < cfg->cmds[i].len; j++) {
+ cptr = line + dsta + (2 * j);
+ ret = raa_dmpvr2_hextou8(cptr, &b);
+ if (ret)
+ goto parse_err;
+ cfg->cmds[i].data[j] = b;
+ }
+ i++;
+ }
+ line = strsep(&buf, "\n");
+ }
+ return 0;
+
+parse_err:
+ kfree(cfg->cmds);
+ return ret;
+}
+
+static int raa_dmpvr2_verify_device(struct raa_dmpvr2_ctrl *ctrl,
+ struct raa_dmpvr2_cfg *cfg)
+{
+ u8 dev_id[5];
+ u8 dev_rev[5];
+ int res;
+
+ res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_ID,
+ 5, dev_id);
+ if (res != 5)
+ return -EIO;
+
+ res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_REV,
+ 5, dev_rev);
+ if (res != 5)
+ return -EIO;
+
+ return memcmp(cfg->dev_id, dev_id+1, 4)
+ | memcmp(cfg->dev_rev+3, dev_rev+4, 1);
+}
+
+static int raa_dmpvr2_check_cfg(struct raa_dmpvr2_ctrl *ctrl,
+ struct raa_dmpvr2_cfg *cfg)
+{
+ u8 data[4];
+
+ i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+ RAA_DMPVR2_NVM_CNT_ADDR);
+ raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ, data);
+ if (cfg->slot_cnt > data[0])
+ return -EINVAL;
+ return 0;
+}
+
+static int raa_dmpvr2_send_cfg(struct raa_dmpvr2_ctrl *ctrl,
+ struct raa_dmpvr2_cfg *cfg)
+{
+ int i, status;
+ u16 word;
+ u32 dbl_word;
+
+ for (i = 0; i < cfg->cmd_cnt; i++) {
+ if (cfg->cmds[i].len == 2) {
+ word = cfg->cmds[i].data[0]
+ | (cfg->cmds[i].data[1] << 8);
+ status = i2c_smbus_write_word_data(ctrl->client,
+ cfg->cmds[i].cmd,
+ word);
+ if (status < 0)
+ return status;
+ } else if (cfg->cmds[i].len == 4) {
+ dbl_word = cfg->cmds[i].data[0]
+ | (cfg->cmds[i].data[1] << 8)
+ | (cfg->cmds[i].data[2] << 16)
+ | (cfg->cmds[i].data[3] << 24);
+ status = raa_dmpvr2_smbus_write32(ctrl->client,
+ cfg->cmds[i].cmd,
+ dbl_word);
+ if (status < 0)
+ return status;
+ } else {
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
+ struct raa_dmpvr2_cfg *cfg)
+{
+ u8 data[4] = {0};
+ u8 data1[4];
+ u8 *dptr;
+ unsigned long start;
+ int i, j, status;
+
+ // Check programmer status
+ start = jiffies;
+ i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+ RAA_DMPVR2_PRGM_STATUS_ADDR);
+ while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
+ raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
+ data);
+ }
+ if (data[0] != 1)
+ return -ETIME;
+
+ // Check bank statuses
+ i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+ RAA_DMPVR2_BANK0_STATUS_ADDR);
+ raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data);
+ i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
+ RAA_DMPVR2_BANK1_STATUS_ADDR);
+ raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data1);
+
+ for (i = 0; i < cfg->slot_cnt; i++) {
+ if (i < 8) {
+ j = i;
+ dptr = data;
+ } else {
+ j = i - 8;
+ dptr = data1;
+ }
+ status = (dptr[j/2] >> (4 * (j % 2))) & 0x0F;
+ if (status != 1)
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static ssize_t raa_dmpvr2_write_cfg(struct raa_dmpvr2_ctrl *ctrl,
+ const char __user *buf, size_t len,
+ loff_t *ppos)
+{
+ char *cbuf;
+ int ret, status;
+ struct raa_dmpvr2_cfg *cfg;
+
+ cfg = kmalloc(sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
+ return -ENOMEM;
+
+ cbuf = kmalloc(len, GFP_KERNEL);
+ if (!cbuf) {
+ status = -ENOMEM;
+ goto buf_err;
+ }
+ ret = simple_write_to_buffer(cbuf, len, ppos, buf, len);
+
+ // Parse file
+ status = raa_dmpvr2_parse_cfg(cbuf, cfg);
+ if (status)
+ goto parse_err;
+ // Verify device and file IDs/revs match
+ status = raa_dmpvr2_verify_device(ctrl, cfg);
+ if (status)
+ goto dev_err;
+ // Verify enough NVM slots available
+ status = raa_dmpvr2_check_cfg(ctrl, cfg);
+ if (status)
+ goto dev_err;
+ // Write CFG to device
+ status = raa_dmpvr2_send_cfg(ctrl, cfg);
+ if (status)
+ goto dev_err;
+ // Verify programming success
+ status = raa_dmpvr2_cfg_write_result(ctrl, cfg);
+ if (status)
+ goto dev_err;
+ // Free memory
+ kfree(cbuf);
+ kfree(cfg->cmds);
+ kfree(cfg);
+ return ret;
+
+ // Handle Errors
+dev_err:
+ kfree(cfg->cmds);
+parse_err:
+ kfree(cbuf);
+buf_err:
+ kfree(cfg);
+ return status;
+}
+
+static ssize_t raa_dmpvr2_debugfs_read(struct file *file, char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ int *idxp = file->private_data;
+ int idx = *idxp;
+ struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
+
+ switch (idx) {
+ case RAA_DMPVR2_DEBUGFS_BB_R:
+ return raa_dmpvr2_read_black_box(ctrl, buf, len, ppos);
+ default:
+ return -EINVAL;
+ }
+}
+
+static ssize_t raa_dmpvr2_debugfs_write(struct file *file,
+ const char __user *buf, size_t len,
+ loff_t *ppos)
+{
+ int *idxp = file->private_data;
+ int idx = *idxp;
+ struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
+
+ switch (idx) {
+ case RAA_DMPVR2_DEBUGFS_CFG_W:
+ return raa_dmpvr2_write_cfg(ctrl, buf, len, ppos);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct file_operations raa_dmpvr2_debugfs_fops = {
+ .llseek = noop_llseek,
+ .read = raa_dmpvr2_debugfs_read,
+ .write = raa_dmpvr2_debugfs_write,
+ .open = simple_open,
+};
+
static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page,
int phase, int reg)
{
@@ -220,7 +640,11 @@ static struct pmbus_driver_info raa_dmpvr_info = {
static int isl68137_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
+ int i, ret;
struct pmbus_driver_info *info;
+ struct dentry *debugfs;
+ struct dentry *debug_dir;
+ struct raa_dmpvr2_ctrl *ctrl;
info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
if (!info)
@@ -262,7 +686,37 @@ static int isl68137_probe(struct i2c_client *client,
return -ENODEV;
}
- return pmbus_do_probe(client, id, info);
+ // No debugfs features for Gen 1
+ if (id->driver_data == raa_dmpvr1_2rail)
+ return pmbus_do_probe(client, id, info);
+
+ ret = pmbus_do_probe(client, id, info);
+ if (ret != 0)
+ return ret;
+
+ ctrl = devm_kzalloc(&client->dev, sizeof(*ctrl), GFP_KERNEL);
+ if (!ctrl)
+ return 0;
+
+ ctrl->client = client;
+ debugfs = pmbus_get_debugfs_dir(client);
+ if (!debugfs)
+ return 0;
+
+ debug_dir = debugfs_create_dir(client->name, debugfs);
+ if (!debug_dir)
+ return 0;
+
+ for (i = 0; i < RAA_DMPVR2_DEBUGFS_NUM_ENTRIES; i++)
+ ctrl->debugfs_entries[i] = i;
+
+ debugfs_create_file("write_config", 0220, debug_dir,
+ &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_CFG_W],
+ &raa_dmpvr2_debugfs_fops);
+ debugfs_create_file("read_black_box", 0440, debug_dir,
+ &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_BB_R],
+ &raa_dmpvr2_debugfs_fops);
+ return 0;
}
static const struct i2c_device_id raa_dmpvr_id[] = {
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] docs: hwmon: Update driver documentation for debugfs
2020-04-22 17:26 [PATCH v2 0/2] hwmon: (pmbus) add debugfs features for Gen 2 Renesas digital multiphase Grant Peltier
2020-04-22 17:27 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints Grant Peltier
@ 2020-04-22 17:27 ` Grant Peltier
1 sibling, 0 replies; 6+ messages in thread
From: Grant Peltier @ 2020-04-22 17:27 UTC (permalink / raw)
To: linux, linux-hwmon, linux-kernel; +Cc: adam.vaughn.xh, grant.peltier.jg
Add usage details for the two new debugfs endpoints for 2nd Gen devices:
read_black_box and write_config
Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
Documentation/hwmon/isl68137.rst | 34 +++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/Documentation/hwmon/isl68137.rst b/Documentation/hwmon/isl68137.rst
index 0e71b22047f8..c5f7b2981db6 100644
--- a/Documentation/hwmon/isl68137.rst
+++ b/Documentation/hwmon/isl68137.rst
@@ -423,7 +423,8 @@ Beyond the normal sysfs pmbus attributes, the driver exposes a control attribute
for the ISL68137.
For 2nd generation Renesas digital multiphase voltage regulators, only the
-normal sysfs pmbus attributes are supported.
+normal sysfs pmbus attributes are supported. However, 2nd generation devices do
+have additional debugfs features which are detailed below.
ISL68137 sysfs attributes
-------------------------
@@ -603,3 +604,34 @@ temp[1-7]_crit_alarm Chip temperature critical high alarm
temp[1-7]_max Maximum temperature
temp[1-7]_max_alarm Chip temperature high alarm
======================= ==========================================
+
+debugfs features (Gen 2 ONLY)
+-----------------------------
+
+If you have debugfs enabled, two file endpoints will be created under
+`/sys/kernel/debug/pmbus/<device-id>` when you instantiate a 2nd generation
+device: `read_black_box` and `write_config`.
+
+`read_black_box` is a read-only file. When read, the driver will attempt to read
+the contents of the device's fault recording RAM segment called "Black Box". The
+RAM segment is then printed to the file as hex formatted strings. For easier
+consumption, the 128-byte segment is broken up into 32 4-byte little-endian
+lines (i.e. each line starts with the least significant byte). See your device's
+datasheet for information regarding how to parse the data.
+
+.. warning::
+ When using `write_config`, only use hex files as they are exported from the
+ Renesas Power Navigator application:
+ https://www.renesas.com/us/en/products/power-management/digital-power/powernavigator.html
+
+`write_config` is a write-only file. This endpoint expects the contents of a
+configuration hex file specific to your device exactly as it is exported from
+the Renesas Power Navigator application. When this endpoint is written to, the
+driver will attempt to write the contents of the file down to one or more of the
+device's NVM slots and will return a standard error code if any part of the
+process fails. Be careful when using this feature as writing an unstable
+configuration to your device may may cause it to misbehave. Each device also has
+a fixed number of NVM writes allowed. Consult the datasheet for your device for
+more information about NVM configuration options. Finally, note that you will
+have to power cycle your device in order for NVM configuration changes to take
+effect.
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints
2020-04-22 17:27 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints Grant Peltier
@ 2020-04-25 17:43 ` Guenter Roeck
2020-04-27 17:07 ` Grant Peltier
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2020-04-25 17:43 UTC (permalink / raw)
To: Grant Peltier; +Cc: linux-hwmon, linux-kernel, adam.vaughn.xh, grant.peltier.jg
On Wed, Apr 22, 2020 at 12:27:14PM -0500, Grant Peltier wrote:
> Add debugfs endpoints to support features of 2nd generation Renesas
> digital multiphase voltage regulators that are not compatible with
> sysfs.
>
> The read_black_box endpoint allows users to read the contents of a
> RAM segment used to record fault conditions within Gen 2 devices.
>
> The write_config endpoint allows users to write configuration hex
> files to Gen 2 devices which modify how they operate.
>
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
Comments inline.
However, the more I look into this, the more concerns I have.
I think we should limit debugfs functions, if they are needed,
to reporting detailed device status. Can you consider handling
configuration from userspace using i2cget / i2cset commands ?
Thanks,
Guenter
> ---
> drivers/hwmon/pmbus/isl68137.c | 460 ++++++++++++++++++++++++++++++++-
> 1 file changed, 457 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
> index 0c622711ef7e..455235a7a56b 100644
> --- a/drivers/hwmon/pmbus/isl68137.c
> +++ b/drivers/hwmon/pmbus/isl68137.c
> @@ -7,10 +7,12 @@
> *
> */
>
> +#include <linux/debugfs.h>
> #include <linux/err.h>
> #include <linux/hwmon-sysfs.h>
> #include <linux/i2c.h>
> #include <linux/init.h>
> +#include <linux/jiffies.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/string.h>
> @@ -18,8 +20,22 @@
>
> #include "pmbus.h"
>
> -#define ISL68137_VOUT_AVS 0x30
> -#define RAA_DMPVR2_READ_VMON 0xc8
> +#define ISL68137_VOUT_AVS 0x30
> +#define RAA_DMPVR2_DMA_FIX 0xc5
> +#define RAA_DMPVR2_DMA_SEQ 0xc6
> +#define RAA_DMPVR2_DMA_ADDR 0xc7
> +#define RAA_DMPVR2_READ_VMON 0xc8
> +#define RAA_DMPVR2_BB_BASE_ADDR 0xef80
> +#define RAA_DMPVR2_BB_WSIZE 4
> +#define RAA_DMPVR2_BB_WCNT 32
> +#define RAA_DMPVR2_BB_BUF_SIZE 288
> +#define RAA_DMPVR2_NVM_CNT_ADDR 0x00c2
> +#define RAA_DMPVR2_PRGM_STATUS_ADDR 0x0707
> +#define RAA_DMPVR2_BANK0_STATUS_ADDR 0x0709
> +#define RAA_DMPVR2_BANK1_STATUS_ADDR 0x070a
> +#define RAA_DMPVR2_CFG_MAX_SLOT 16
> +#define RAA_DMPVR2_CFG_HEAD_LEN 290
> +#define RAA_DMPVR2_CFG_SLOT_LEN 358
>
> enum chips {
> isl68137,
> @@ -71,6 +87,21 @@ enum variants {
> raa_dmpvr2_hv,
> };
>
> +enum {
> + RAA_DMPVR2_DEBUGFS_CFG_W = 0,
> + RAA_DMPVR2_DEBUGFS_BB_R,
> + RAA_DMPVR2_DEBUGFS_NUM_ENTRIES,
> +};
> +
> +struct raa_dmpvr2_ctrl {
> + enum chips part;
> + struct i2c_client *client;
> + int debugfs_entries[RAA_DMPVR2_DEBUGFS_NUM_ENTRIES];
> +};
> +
> +#define to_ctrl(x, y) container_of((x), struct raa_dmpvr2_ctrl, \
> + debugfs_entries[(y)])
> +
> static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
> int page,
> char *buf)
> @@ -157,6 +188,395 @@ static const struct attribute_group *isl68137_attribute_groups[] = {
> NULL,
> };
>
> +/**
> + * Helper function required since linux SMBus implementation does not currently
> + * (v5.6) support the SMBus 3.0 "Read 32" protocol
> + */
> +static s32 raa_dmpvr2_smbus_read32(const struct i2c_client *client, u8 command,
> + unsigned char *data)
> +{
> + int status;
> + unsigned char msgbuf[1];
> + struct i2c_msg msg[2] = {
> + {
> + .addr = client->addr,
> + .flags = client->flags,
> + .len = 1,
> + .buf = msgbuf,
> + },
> + {
> + .addr = client->addr,
> + .flags = client->flags | I2C_M_RD,
> + .len = 4,
> + .buf = data,
> + },
> + };
> +
> + msgbuf[0] = command;
> + status = i2c_transfer(client->adapter, msg, 2);
> + if (status != 2)
> + return status;
> + return 0;
The return value from this function is not checked. Either check it,
or don't return a value.
> +}
> +
> +/**
> + * Helper function required since linux SMBus implementation does not currently
> + * (v5.6) support the SMBus 3.0 "Write 32" protocol
> + */
> +static s32 raa_dmpvr2_smbus_write32(const struct i2c_client *client,
> + u8 command, u32 value)
> +{
> + int status;
> + unsigned char msgbuf[5];
> + struct i2c_msg msg[1] = {
> + {
> + .addr = client->addr,
> + .flags = client->flags,
> + .len = 5,
> + .buf = msgbuf,
> + },
> + };
> + msgbuf[0] = command;
> + msgbuf[1] = value & 0x0FF;
> + msgbuf[2] = (value >> 8) & 0x0FF;
> + msgbuf[3] = (value >> 16) & 0x0FF;
> + msgbuf[4] = (value >> 24) & 0x0FF;
> + status = i2c_transfer(client->adapter, msg, 1);
> + if (status != 1)
> + return status;
> + return 0;
> +}
> +
> +static ssize_t raa_dmpvr2_read_black_box(struct raa_dmpvr2_ctrl *ctrl,
> + char __user *buf, size_t len,
> + loff_t *ppos)
> +{
> + int i, j;
> + u16 addr = RAA_DMPVR2_BB_BASE_ADDR;
> + unsigned char data[RAA_DMPVR2_BB_WSIZE] = { 0 };
> + char out[RAA_DMPVR2_BB_BUF_SIZE] = { 0 };
> + char *optr = out;
> +
> + i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR, addr);
> + for (i = 0; i < RAA_DMPVR2_BB_WCNT; i++) {
> + raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ,
> + data);
> + for (j = 0; j < RAA_DMPVR2_BB_WSIZE; j++)
> + optr += snprintf(optr, sizeof(out), "%02X", data[j]);
The length check here only ensures that a single call to snprintf() doesn't add
more data to the buffer than available. However, since optr is advanced, there
is no practical limit on the total data written.
I understand that the maximum amount of data written in practice
is RAA_DMPVR2_BB_WCNT * ((RAA_DMPVR2_BB_WSIZE * 2) + 1), or
32 * ((4 * 2) + 1) = 288, meaning the buffer is large enough, but
using snprintf() with sizeof(out) to limit the length of data written
is pretty pointless. Somewhat cleaner code might keep track of the length
written, write to out + length, and pass sizeof(out) - length as parameter.
> + *optr = '\n';
> + optr++;
> + }
> +
> + return simple_read_from_buffer(buf, len, ppos, &out, sizeof(out));
> +}
> +
> +struct raa_dmpvr_cfg_cmd {
> + u8 cmd;
> + u8 len;
> + u8 data[4];
> +};
> +
> +struct raa_dmpvr2_cfg {
> + u8 dev_id[4];
> + u8 dev_rev[4];
> + int slot_cnt;
> + int cmd_cnt;
> + struct raa_dmpvr_cfg_cmd *cmds;
> + u8 crc[4];
> +};
> +
> +/**
> + * Helper function to handle hex string to byte conversions
> + */
> +static int raa_dmpvr2_hextou8(char *buf, u8 *res)
> +{
> + char s[3];
> +
> + s[0] = buf[0];
> + s[1] = buf[1];
> + s[2] = '\0';
> + return kstrtou8(s, 16, res);
> +}
> +
> +static int raa_dmpvr2_parse_cfg(char *buf, struct raa_dmpvr2_cfg *cfg)
> +{
> + const int lsta = 2;
> + const int csta = 6;
> + const int dsta = 8;
> + char *cptr, *line;
> + int lcnt = 1;
> + int ccnt = 0;
> + int i, j, ret;
> + u8 b;
> +
> + // Ensure there is enough memory for the file
Please don't mix c++ comments into a driver using c style comments.
> + for (i = 0; i < strlen(buf); i++) {
> + if (buf[i] == '\n' && i < strlen(buf)-2) {
> + lcnt++;
> + if (buf[i+1] != '4' || buf[i+2] != '9')
Space around operators, please.
> + ccnt++;
> + }
> + }
> + cfg->slot_cnt = (lcnt-RAA_DMPVR2_CFG_HEAD_LEN)/RAA_DMPVR2_CFG_SLOT_LEN;
> + if (cfg->slot_cnt < 1 || cfg->slot_cnt > RAA_DMPVR2_CFG_MAX_SLOT)
> + return -EINVAL;
> + cfg->cmd_cnt = ccnt;
> + cfg->cmds = kmalloc_array(ccnt, sizeof(struct raa_dmpvr_cfg_cmd),
> + GFP_KERNEL);
> + if (!cfg->cmds)
> + return -ENOMEM;
> +
> + // Parse header
> + for (i = 0; i < 2; i++) {
> + line = strsep(&buf, "\n");
> + cptr = line + dsta;
> + for (j = 3; j >= 0; j--) {
> + ret = raa_dmpvr2_hextou8(cptr, &b);
> + if (ret)
> + goto parse_err;
> + if (i == 0)
> + cfg->dev_id[j] = b;
> + else
> + cfg->dev_rev[j] = b;
> + cptr += 2;
> + }
> + }
> +
> + // Parse cmds
> + i = 0;
> + while (line != NULL && strlen(line) > (dsta + 2)) {
> + if (strncmp(line, "49", 2) != 0) {
> + ret = raa_dmpvr2_hextou8(line+lsta, &b);
> + if (ret)
> + goto parse_err;
> + cfg->cmds[i].len = b - 3;
> + ret = raa_dmpvr2_hextou8(line+csta, &b);
> + if (ret)
> + goto parse_err;
> + cfg->cmds[i].cmd = b;
> + for (j = 0; j < cfg->cmds[i].len; j++) {
> + cptr = line + dsta + (2 * j);
> + ret = raa_dmpvr2_hextou8(cptr, &b);
> + if (ret)
> + goto parse_err;
> + cfg->cmds[i].data[j] = b;
> + }
> + i++;
> + }
> + line = strsep(&buf, "\n");
> + }
> + return 0;
> +
> +parse_err:
> + kfree(cfg->cmds);
> + return ret;
> +}
> +
> +static int raa_dmpvr2_verify_device(struct raa_dmpvr2_ctrl *ctrl,
> + struct raa_dmpvr2_cfg *cfg)
> +{
> + u8 dev_id[5];
> + u8 dev_rev[5];
> + int res;
> +
> + res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_ID,
> + 5, dev_id);
> + if (res != 5)
> + return -EIO;
The calling code returns this as error code.
> +
> + res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_REV,
> + 5, dev_rev);
It still puzzles me, quite frankly, why i2c_smbus_read_block_data()
would not work here.
> + if (res != 5)
> + return -EIO;
> +
> + return memcmp(cfg->dev_id, dev_id+1, 4)
> + | memcmp(cfg->dev_rev+3, dev_rev+4, 1);
Please no arithmetic or of return codes. Besides, this is dangerous:
A mismatch returns a value <0 or >0 depending on the match, and this
is then returned as valid (and pretty much random) error code.
This function should return -ENODEV on error.
> +}
> +
> +static int raa_dmpvr2_check_cfg(struct raa_dmpvr2_ctrl *ctrl,
> + struct raa_dmpvr2_cfg *cfg)
> +{
> + u8 data[4];
> +
> + i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> + RAA_DMPVR2_NVM_CNT_ADDR);
> + raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ, data);
> + if (cfg->slot_cnt > data[0])
If raa_dmpvr2_smbus_read32 returns an error, the content of data[] is
random. In that case, this code is checking a random value.
> + return -EINVAL;
> + return 0;
> +}
> +
> +static int raa_dmpvr2_send_cfg(struct raa_dmpvr2_ctrl *ctrl,
> + struct raa_dmpvr2_cfg *cfg)
> +{
> + int i, status;
> + u16 word;
> + u32 dbl_word;
> +
> + for (i = 0; i < cfg->cmd_cnt; i++) {
> + if (cfg->cmds[i].len == 2) {
> + word = cfg->cmds[i].data[0]
> + | (cfg->cmds[i].data[1] << 8);
> + status = i2c_smbus_write_word_data(ctrl->client,
> + cfg->cmds[i].cmd,
> + word);
> + if (status < 0)
> + return status;
> + } else if (cfg->cmds[i].len == 4) {
> + dbl_word = cfg->cmds[i].data[0]
> + | (cfg->cmds[i].data[1] << 8)
> + | (cfg->cmds[i].data[2] << 16)
> + | (cfg->cmds[i].data[3] << 24);
> + status = raa_dmpvr2_smbus_write32(ctrl->client,
> + cfg->cmds[i].cmd,
> + dbl_word);
> + if (status < 0)
> + return status;
> + } else {
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
> + struct raa_dmpvr2_cfg *cfg)
> +{
> + u8 data[4] = {0};
> + u8 data1[4];
> + u8 *dptr;
> + unsigned long start;
> + int i, j, status;
> +
> + // Check programmer status
> + start = jiffies;
> + i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> + RAA_DMPVR2_PRGM_STATUS_ADDR);
> + while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
> + raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
> + data);
> + }
> + if (data[0] != 1)
> + return -ETIME;
Are you sure ? Normally I would expect ETIMEDOUT.
> +
> + // Check bank statuses
> + i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> + RAA_DMPVR2_BANK0_STATUS_ADDR);
> + raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data);
> + i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> + RAA_DMPVR2_BANK1_STATUS_ADDR);
> + raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data1);
> +
> + for (i = 0; i < cfg->slot_cnt; i++) {
> + if (i < 8) {
> + j = i;
> + dptr = data;
> + } else {
> + j = i - 8;
> + dptr = data1;
> + }
> + status = (dptr[j/2] >> (4 * (j % 2))) & 0x0F;
> + if (status != 1)
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t raa_dmpvr2_write_cfg(struct raa_dmpvr2_ctrl *ctrl,
> + const char __user *buf, size_t len,
> + loff_t *ppos)
> +{
> + char *cbuf;
> + int ret, status;
> + struct raa_dmpvr2_cfg *cfg;
> +
> + cfg = kmalloc(sizeof(*cfg), GFP_KERNEL);
> + if (!cfg)
> + return -ENOMEM;
> +
> + cbuf = kmalloc(len, GFP_KERNEL);
> + if (!cbuf) {
> + status = -ENOMEM;
> + goto buf_err;
> + }
> + ret = simple_write_to_buffer(cbuf, len, ppos, buf, len);
> +
> + // Parse file
> + status = raa_dmpvr2_parse_cfg(cbuf, cfg);
> + if (status)
> + goto parse_err;
> + // Verify device and file IDs/revs match
> + status = raa_dmpvr2_verify_device(ctrl, cfg);
> + if (status)
> + goto dev_err;
> + // Verify enough NVM slots available
> + status = raa_dmpvr2_check_cfg(ctrl, cfg);
> + if (status)
> + goto dev_err;
> + // Write CFG to device
> + status = raa_dmpvr2_send_cfg(ctrl, cfg);
> + if (status)
> + goto dev_err;
> + // Verify programming success
> + status = raa_dmpvr2_cfg_write_result(ctrl, cfg);
> + if (status)
> + goto dev_err;
> + // Free memory
That comment is a bit useless.
> + kfree(cbuf);
> + kfree(cfg->cmds);
> + kfree(cfg);
> + return ret;
> +
> + // Handle Errors
Another useless comment.
> +dev_err:
> + kfree(cfg->cmds);
> +parse_err:
> + kfree(cbuf);
> +buf_err:
> + kfree(cfg);
> + return status;
> +}
> +
> +static ssize_t raa_dmpvr2_debugfs_read(struct file *file, char __user *buf,
> + size_t len, loff_t *ppos)
> +{
> + int *idxp = file->private_data;
> + int idx = *idxp;
> + struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
> +
> + switch (idx) {
> + case RAA_DMPVR2_DEBUGFS_BB_R:
> + return raa_dmpvr2_read_black_box(ctrl, buf, len, ppos);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t raa_dmpvr2_debugfs_write(struct file *file,
> + const char __user *buf, size_t len,
> + loff_t *ppos)
> +{
> + int *idxp = file->private_data;
> + int idx = *idxp;
> + struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
> +
> + switch (idx) {
> + case RAA_DMPVR2_DEBUGFS_CFG_W:
> + return raa_dmpvr2_write_cfg(ctrl, buf, len, ppos);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct file_operations raa_dmpvr2_debugfs_fops = {
> + .llseek = noop_llseek,
> + .read = raa_dmpvr2_debugfs_read,
> + .write = raa_dmpvr2_debugfs_write,
> + .open = simple_open,
> +};
> +
> static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page,
> int phase, int reg)
> {
> @@ -220,7 +640,11 @@ static struct pmbus_driver_info raa_dmpvr_info = {
> static int isl68137_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> + int i, ret;
> struct pmbus_driver_info *info;
> + struct dentry *debugfs;
> + struct dentry *debug_dir;
> + struct raa_dmpvr2_ctrl *ctrl;
>
> info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
> if (!info)
> @@ -262,7 +686,37 @@ static int isl68137_probe(struct i2c_client *client,
> return -ENODEV;
> }
>
> - return pmbus_do_probe(client, id, info);
> + // No debugfs features for Gen 1
> + if (id->driver_data == raa_dmpvr1_2rail)
> + return pmbus_do_probe(client, id, info);
> +
> + ret = pmbus_do_probe(client, id, info);
> + if (ret != 0)
> + return ret;
Why not do the version check and return here ?
if (ret || id->driver_data == raa_dmpvr1_2rail)
return ret;
> +
> + ctrl = devm_kzalloc(&client->dev, sizeof(*ctrl), GFP_KERNEL);
> + if (!ctrl)
> + return 0;
> +
> + ctrl->client = client;
> + debugfs = pmbus_get_debugfs_dir(client);
> + if (!debugfs)
> + return 0;
> +
> + debug_dir = debugfs_create_dir(client->name, debugfs);
> + if (!debug_dir)
> + return 0;
> +
> + for (i = 0; i < RAA_DMPVR2_DEBUGFS_NUM_ENTRIES; i++)
> + ctrl->debugfs_entries[i] = i;
> +
> + debugfs_create_file("write_config", 0220, debug_dir,
> + &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_CFG_W],
> + &raa_dmpvr2_debugfs_fops);
> + debugfs_create_file("read_black_box", 0440, debug_dir,
> + &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_BB_R],
> + &raa_dmpvr2_debugfs_fops);
> + return 0;
> }
>
> static const struct i2c_device_id raa_dmpvr_id[] = {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints
2020-04-25 17:43 ` Guenter Roeck
@ 2020-04-27 17:07 ` Grant Peltier
2020-04-29 14:11 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Grant Peltier @ 2020-04-27 17:07 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, linux-kernel, adam.vaughn.xh, grant.peltier.jg
On Sat, Apr 25, 2020 at 10:43:18AM -0700, Guenter Roeck wrote:
> On Wed, Apr 22, 2020 at 12:27:14PM -0500, Grant Peltier wrote:
> > Add debugfs endpoints to support features of 2nd generation Renesas
> > digital multiphase voltage regulators that are not compatible with
> > sysfs.
> >
> > The read_black_box endpoint allows users to read the contents of a
> > RAM segment used to record fault conditions within Gen 2 devices.
> >
> > The write_config endpoint allows users to write configuration hex
> > files to Gen 2 devices which modify how they operate.
> >
> > Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
>
> Comments inline.
>
> However, the more I look into this, the more concerns I have.
> I think we should limit debugfs functions, if they are needed,
> to reporting detailed device status. Can you consider handling
> configuration from userspace using i2cget / i2cset commands ?
The reason we decided to try to implement configuration writes within the
driver is that we found a userspace implementation to be unstable. The
process requires anywhere from approximately 650 to a few thousand 32-bit
writes (dependent on number of NVM slots contained in the file). The entire
write process therefore takes a non-trivial amount of CPU time to complete
and the userspace process was often interrupted which would cause for it
to fail. Writing the configuration directly from the driver has been less
error prone.
> > + res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_REV,
> > + 5, dev_rev);
>
> It still puzzles me, quite frankly, why i2c_smbus_read_block_data()
> would not work here.
>
i2c_smbus_read_block_data() requires the underlying driver/controller to handle
interpretting the initial length byte read from the client device and then
continuing to read that number of bytes. Not all controllers (e.g. BCM2835)
support this. On the other hand, i2c_smbus_read_i2c_block_data() just does a
fixed-length read based on the given length parameter.
> > +static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
> > + struct raa_dmpvr2_cfg *cfg)
> > +{
> > + u8 data[4] = {0};
> > + u8 data1[4];
> > + u8 *dptr;
> > + unsigned long start;
> > + int i, j, status;
> > +
> > + // Check programmer status
> > + start = jiffies;
> > + i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> > + RAA_DMPVR2_PRGM_STATUS_ADDR);
> > + while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
> > + raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
> > + data);
> > + }
> > + if (data[0] != 1)
> > + return -ETIME;
>
> Are you sure ? Normally I would expect ETIMEDOUT.
My understanding is that ETIME is meant for timer expiration whereas ETIMEDOUT
is meant for connection timeout errors. Is that correct? In this case, we are
not really waiting on the device to respond but instead are constantly polling
until the device responds with the desired value. However, I can understand an
argument for ETIMEDOUT here and can swtich to that if you think it is more
appropriate.
Thank you for your other notes. I will refactor as requested.
Grant
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints
2020-04-27 17:07 ` Grant Peltier
@ 2020-04-29 14:11 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2020-04-29 14:11 UTC (permalink / raw)
To: Grant Peltier; +Cc: linux-hwmon, linux-kernel, adam.vaughn.xh, grant.peltier.jg
On Mon, Apr 27, 2020 at 12:07:29PM -0500, Grant Peltier wrote:
> On Sat, Apr 25, 2020 at 10:43:18AM -0700, Guenter Roeck wrote:
> > On Wed, Apr 22, 2020 at 12:27:14PM -0500, Grant Peltier wrote:
> > > Add debugfs endpoints to support features of 2nd generation Renesas
> > > digital multiphase voltage regulators that are not compatible with
> > > sysfs.
> > >
> > > The read_black_box endpoint allows users to read the contents of a
> > > RAM segment used to record fault conditions within Gen 2 devices.
> > >
> > > The write_config endpoint allows users to write configuration hex
> > > files to Gen 2 devices which modify how they operate.
> > >
> > > Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> >
> > Comments inline.
> >
> > However, the more I look into this, the more concerns I have.
> > I think we should limit debugfs functions, if they are needed,
> > to reporting detailed device status. Can you consider handling
> > configuration from userspace using i2cget / i2cset commands ?
>
> The reason we decided to try to implement configuration writes within the
> driver is that we found a userspace implementation to be unstable. The
> process requires anywhere from approximately 650 to a few thousand 32-bit
> writes (dependent on number of NVM slots contained in the file). The entire
> write process therefore takes a non-trivial amount of CPU time to complete
> and the userspace process was often interrupted which would cause for it
> to fail. Writing the configuration directly from the driver has been less
> error prone.
>
The downside is that anyone can easily mess with the chip. That does make
me quite concerned - I have seen many PMBus chips and even boards blown up
if misconfigured. It is bad enough that this can be done in the first place,
but I don't want to make it even easier by providing the means to do so via
debugfs files.
Did you try with an application sending ioctls directly ? That should
be much less error prone than i2cset/i2cget. Also, a userspace process
like this should have appropriate priorities (ie run as realtime process)
to avoid being preempted.
Thanks,
Guenter
> > > + res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_REV,
> > > + 5, dev_rev);
> >
> > It still puzzles me, quite frankly, why i2c_smbus_read_block_data()
> > would not work here.
> >
>
> i2c_smbus_read_block_data() requires the underlying driver/controller to handle
> interpretting the initial length byte read from the client device and then
> continuing to read that number of bytes. Not all controllers (e.g. BCM2835)
> support this. On the other hand, i2c_smbus_read_i2c_block_data() just does a
> fixed-length read based on the given length parameter.
>
> > > +static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
> > > + struct raa_dmpvr2_cfg *cfg)
> > > +{
> > > + u8 data[4] = {0};
> > > + u8 data1[4];
> > > + u8 *dptr;
> > > + unsigned long start;
> > > + int i, j, status;
> > > +
> > > + // Check programmer status
> > > + start = jiffies;
> > > + i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> > > + RAA_DMPVR2_PRGM_STATUS_ADDR);
> > > + while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
> > > + raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
> > > + data);
> > > + }
> > > + if (data[0] != 1)
> > > + return -ETIME;
> >
> > Are you sure ? Normally I would expect ETIMEDOUT.
>
> My understanding is that ETIME is meant for timer expiration whereas ETIMEDOUT
> is meant for connection timeout errors. Is that correct? In this case, we are
> not really waiting on the device to respond but instead are constantly polling
> until the device responds with the desired value. However, I can understand an
> argument for ETIMEDOUT here and can swtich to that if you think it is more
> appropriate.
>
>
> Thank you for your other notes. I will refactor as requested.
>
> Grant
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-29 14:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-22 17:26 [PATCH v2 0/2] hwmon: (pmbus) add debugfs features for Gen 2 Renesas digital multiphase Grant Peltier
2020-04-22 17:27 ` [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints Grant Peltier
2020-04-25 17:43 ` Guenter Roeck
2020-04-27 17:07 ` Grant Peltier
2020-04-29 14:11 ` Guenter Roeck
2020-04-22 17:27 ` [PATCH v2 2/2] docs: hwmon: Update driver documentation for debugfs Grant Peltier
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).