| Message ID | acbdc26389b6ab5f5f4536de7332b03c45a95f00.1587572415.git.grantpeltier93@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (pmbus) add debugfs features for Gen 2 Renesas digital multiphase | expand |
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[] = {
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
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
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[] = {
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(-)