This is a series of patches that fixes up Rainier and Everest systems. Some of them are considered workarounds, and are not ready for upstream linux. Others are simply support for older system hardware that doesn't need to go to the mainline tree. Andrew Geissler (1): fsi: run clock at 100MHz Andrew Jeffery (6): i2c: Allow throttling of transfers to client devices pmbus: (ucd9000) Throttle SMBus transfers to avoid poor behaviour ucd9000: Add a throttle delay attribute in debugfs pmbus: (core) Add a one-shot retry in pmbus_set_page() pmbus: (max31785) Add a local pmbus_set_page() implementation pmbus: (max31785) Retry enabling fans after writing MFR_FAN_CONFIG Eddie James (7): ARM: dts: aspeed: Rainier: Add fan controller properties ARM: dts: aspeed: Everest: Add fan controller properties ARM: dts: aspeed: Rainier 4U: Delete fan dual-tach properties ARM: dts: aspeed: Add Rainier 2U and 4U device trees for pass 1 hardware fsi: sbefifo: Increase command timeouts to 30 seconds fsi: occ: Add dynamic debug to dump command and response fsi: sbefifo: Use interruptible mutex locking arch/arm/boot/dts/Makefile | 2 + arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts | 28 +++ .../boot/dts/aspeed-bmc-ibm-rainier-4u-p1.dts | 94 ++++++++++ .../boot/dts/aspeed-bmc-ibm-rainier-4u.dts | 24 +++ .../boot/dts/aspeed-bmc-ibm-rainier-p1.dts | 94 ++++++++++ arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 42 +++++ drivers/fsi/fsi-master-aspeed.c | 3 +- drivers/fsi/fsi-occ.c | 44 +++++ drivers/fsi/fsi-sbefifo.c | 18 +- drivers/hwmon/pmbus/max31785.c | 55 ++++-- drivers/hwmon/pmbus/pmbus_core.c | 31 ++-- drivers/hwmon/pmbus/ucd9000.c | 39 ++++ drivers/i2c/i2c-core-base.c | 8 +- drivers/i2c/i2c-core-smbus.c | 169 +++++++++++++++--- drivers/i2c/i2c-core.h | 21 +++ include/linux/i2c.h | 5 + 16 files changed, 618 insertions(+), 59 deletions(-) create mode 100644 arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u-p1.dts create mode 100644 arch/arm/boot/dts/aspeed-bmc-ibm-rainier-p1.dts -- 2.27.0
From: Andrew Jeffery <andrew@aj.id.au> Some devices fail to cope in disastrous ways with the small command turn-around times enabled by in-kernel device drivers. Introduce per-client throttling of transfers to avoid these issues. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/i2c/i2c-core-base.c | 8 +- drivers/i2c/i2c-core-smbus.c | 169 +++++++++++++++++++++++++++++------ drivers/i2c/i2c-core.h | 21 +++++ include/linux/i2c.h | 5 ++ 4 files changed, 172 insertions(+), 31 deletions(-) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index bdce6d3e5327..bf35710d0407 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -871,13 +871,17 @@ int i2c_dev_irq_from_resources(const struct resource *resources, struct i2c_client * i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info) { + struct i2c_client_priv *priv; struct i2c_client *client; int status; - client = kzalloc(sizeof *client, GFP_KERNEL); - if (!client) + priv = kzalloc(sizeof *priv, GFP_KERNEL); + if (!priv) return ERR_PTR(-ENOMEM); + mutex_init(&priv->throttle_lock); + client = &priv->client; + client->adapter = adap; client->dev.platform_data = info->platform_data; diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c index f5c9787992e9..1dde58c8a387 100644 --- a/drivers/i2c/i2c-core-smbus.c +++ b/drivers/i2c/i2c-core-smbus.c @@ -10,6 +10,7 @@ * SMBus 2.0 support by Mark Studebaker <mdsxyz123@yahoo.com> and * Jean Delvare <jdelvare@suse.de> */ +#include <linux/delay.h> #include <linux/device.h> #include <linux/err.h> #include <linux/i2c.h> @@ -21,6 +22,9 @@ #define CREATE_TRACE_POINTS #include <trace/events/smbus.h> +static s32 i2c_smbus_throttle_xfer(const struct i2c_client *client, + char read_write, u8 command, int protocol, + union i2c_smbus_data *data); /* The SMBus parts */ @@ -95,9 +99,9 @@ s32 i2c_smbus_read_byte(const struct i2c_client *client) union i2c_smbus_data data; int status; - status = i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_READ, 0, - I2C_SMBUS_BYTE, &data); + status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE, &data); + return (status < 0) ? status : data.byte; } EXPORT_SYMBOL(i2c_smbus_read_byte); @@ -112,8 +116,8 @@ EXPORT_SYMBOL(i2c_smbus_read_byte); */ s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value) { - return i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL); + return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, value, + I2C_SMBUS_BYTE, NULL); } EXPORT_SYMBOL(i2c_smbus_write_byte); @@ -130,9 +134,9 @@ s32 i2c_smbus_read_byte_data(const struct i2c_client *client, u8 command) union i2c_smbus_data data; int status; - status = i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_READ, command, - I2C_SMBUS_BYTE_DATA, &data); + status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command, + I2C_SMBUS_BYTE_DATA, &data); + return (status < 0) ? status : data.byte; } EXPORT_SYMBOL(i2c_smbus_read_byte_data); @@ -150,10 +154,10 @@ s32 i2c_smbus_write_byte_data(const struct i2c_client *client, u8 command, u8 value) { union i2c_smbus_data data; + data.byte = value; - return i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_WRITE, command, - I2C_SMBUS_BYTE_DATA, &data); + return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command, + I2C_SMBUS_BYTE_DATA, &data); } EXPORT_SYMBOL(i2c_smbus_write_byte_data); @@ -170,9 +174,9 @@ s32 i2c_smbus_read_word_data(const struct i2c_client *client, u8 command) union i2c_smbus_data data; int status; - status = i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_READ, command, - I2C_SMBUS_WORD_DATA, &data); + status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command, + I2C_SMBUS_WORD_DATA, &data); + return (status < 0) ? status : data.word; } EXPORT_SYMBOL(i2c_smbus_read_word_data); @@ -190,10 +194,10 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command, u16 value) { union i2c_smbus_data data; + data.word = value; - return i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_WRITE, command, - I2C_SMBUS_WORD_DATA, &data); + return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command, + I2C_SMBUS_WORD_DATA, &data); } EXPORT_SYMBOL(i2c_smbus_write_word_data); @@ -218,9 +222,9 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command, union i2c_smbus_data data; int status; - status = i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_READ, command, - I2C_SMBUS_BLOCK_DATA, &data); + status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command, + I2C_SMBUS_BLOCK_DATA, &data); + if (status) return status; @@ -248,9 +252,8 @@ s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command, length = I2C_SMBUS_BLOCK_MAX; data.block[0] = length; memcpy(&data.block[1], values, length); - return i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_WRITE, command, - I2C_SMBUS_BLOCK_DATA, &data); + return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command, + I2C_SMBUS_BLOCK_DATA, &data); } EXPORT_SYMBOL(i2c_smbus_write_block_data); @@ -264,9 +267,9 @@ s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command, if (length > I2C_SMBUS_BLOCK_MAX) length = I2C_SMBUS_BLOCK_MAX; data.block[0] = length; - status = i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_READ, command, - I2C_SMBUS_I2C_BLOCK_DATA, &data); + status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command, + I2C_SMBUS_I2C_BLOCK_DATA, &data); + if (status < 0) return status; @@ -284,9 +287,8 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command, length = I2C_SMBUS_BLOCK_MAX; data.block[0] = length; memcpy(data.block + 1, values, length); - return i2c_smbus_xfer(client->adapter, client->addr, client->flags, - I2C_SMBUS_WRITE, command, - I2C_SMBUS_I2C_BLOCK_DATA, &data); + return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command, + I2C_SMBUS_I2C_BLOCK_DATA, &data); } EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data); @@ -547,6 +549,71 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, } EXPORT_SYMBOL(i2c_smbus_xfer); +static int i2c_smbus_throttle_enter(const struct i2c_client *client) + __acquires(&priv->throttle_lock) +{ + struct i2c_client_priv *priv; + ktime_t earliest; + int rc; + + priv = to_i2c_client_priv(client); + + if (i2c_in_atomic_xfer_mode()) { + if (!mutex_trylock(&priv->throttle_lock)) + return -EAGAIN; + } else { + rc = mutex_lock_interruptible(&priv->throttle_lock); + if (rc) + return rc; + } + earliest = ktime_add_us(priv->last, priv->delay_us); + + if (priv->delay_us && ktime_before(ktime_get(), earliest)) { + if (i2c_in_atomic_xfer_mode()) { + mutex_unlock(&priv->throttle_lock); + return -EAGAIN; + } + + usleep_range(priv->delay_us, 2 * priv->delay_us); + } + + return 0; +} + +static void i2c_smbus_throttle_exit(const struct i2c_client *client) + __releases(&priv->throttle_lock) +{ + struct i2c_client_priv *priv; + + priv = to_i2c_client_priv(client); + + if (priv->delay_us) + priv->last = ktime_get(); + mutex_unlock(&priv->throttle_lock); +} + +static s32 i2c_smbus_throttle_xfer(const struct i2c_client *client, + char read_write, u8 command, int protocol, + union i2c_smbus_data *data) +{ + s32 res; + + res = i2c_smbus_throttle_enter(client); + if (res) + return res; + + res = __i2c_lock_bus_helper(client->adapter); + if (!res) + res = __i2c_smbus_xfer(client->adapter, client->addr, + client->flags, read_write, command, + protocol, data); + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + + i2c_smbus_throttle_exit(client); + + return res; +} + s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags, char read_write, u8 command, int protocol, union i2c_smbus_data *data) @@ -715,3 +782,47 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert); #endif + +int i2c_smbus_throttle_client(struct i2c_client *client, + unsigned long delay_us) +{ + struct i2c_client_priv *priv; + int rc; + + priv = to_i2c_client_priv(client); + + if (i2c_in_atomic_xfer_mode()) { + if (!mutex_trylock(&priv->throttle_lock)) + return -EAGAIN; + } else { + rc = mutex_lock_interruptible(&priv->throttle_lock); + if (rc) + return rc; + } + priv->delay_us = delay_us; + priv->last = ktime_get(); + mutex_unlock(&priv->throttle_lock); + + return 0; +} + +int i2c_smbus_throttle_value(struct i2c_client *client, unsigned long *delay_us) +{ + struct i2c_client_priv *priv; + int rc; + + priv = to_i2c_client_priv(client); + + if (i2c_in_atomic_xfer_mode()) { + if (!mutex_trylock(&priv->throttle_lock)) + return -EAGAIN; + } else { + rc = mutex_lock_interruptible(&priv->throttle_lock); + if (rc) + return rc; + } + *delay_us = priv->delay_us; + mutex_unlock(&priv->throttle_lock); + + return 0; +} diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h index 8ce261167a2d..e916624c7b98 100644 --- a/drivers/i2c/i2c-core.h +++ b/drivers/i2c/i2c-core.h @@ -4,6 +4,27 @@ */ #include <linux/rwsem.h> +#include <linux/i2c.h> +#include <linux/timekeeping.h> +#include <linux/kernel.h> +#include <linux/mutex.h> + +struct i2c_client_priv { + struct i2c_client client; + + /* + * Per-client access throttling, described in terms of microsecond + * delay between the end of the nth transfer and the start of the + * (n+1)th transfer + * + * Do it in a wrapper struct to preserve const-ness of the i2c_smbus_* + * interfaces. + */ + struct mutex throttle_lock; + unsigned long delay_us; + ktime_t last; +}; +#define to_i2c_client_priv(c) container_of(c, struct i2c_client_priv, client) struct i2c_devinfo { struct list_head list; diff --git a/include/linux/i2c.h b/include/linux/i2c.h index a670ae129f4b..89dbe147040f 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -181,8 +181,13 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client, u8 command, u8 length, u8 *values); +int i2c_smbus_throttle_client(struct i2c_client *client, + unsigned long delay_us); +int i2c_smbus_throttle_value(struct i2c_client *client, + unsigned long *delay_us); int i2c_get_device_id(const struct i2c_client *client, struct i2c_device_identity *id); + #endif /* I2C */ /** -- 2.27.0
From: Andrew Jeffery <andrew@aj.id.au> Short turn-around times between transfers to UCD9000 devices can lead to problematic behaviour, including unnecessary clock stretching, bus lockups and potential corruption of the device's volatile state. Introduce transfer throttling for the device with a minimum access delay of 1ms. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/hwmon/pmbus/ucd9000.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c index a15e6fe3e425..9687bc3609d1 100644 --- a/drivers/hwmon/pmbus/ucd9000.c +++ b/drivers/hwmon/pmbus/ucd9000.c @@ -487,6 +487,8 @@ static int ucd9000_init_debugfs(struct i2c_client *client, } #endif /* CONFIG_DEBUG_FS */ +#define UCD9000_SMBUS_THROTTLE_US 1000 + static int ucd9000_probe(struct i2c_client *client) { u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1]; @@ -501,6 +503,8 @@ static int ucd9000_probe(struct i2c_client *client) I2C_FUNC_SMBUS_BLOCK_DATA)) return -ENODEV; + i2c_smbus_throttle_client(client, UCD9000_SMBUS_THROTTLE_US); + ret = i2c_smbus_read_block_data(client, UCD9000_DEVICE_ID, block_buffer); if (ret < 0) { -- 2.27.0
From: Andrew Jeffery <andrew@aj.id.au> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/hwmon/pmbus/ucd9000.c | 37 ++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c index 9687bc3609d1..cbfafebf1ded 100644 --- a/drivers/hwmon/pmbus/ucd9000.c +++ b/drivers/hwmon/pmbus/ucd9000.c @@ -401,6 +401,37 @@ static int ucd9000_debugfs_show_mfr_status_bit(void *data, u64 *val) DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_bit, ucd9000_debugfs_show_mfr_status_bit, NULL, "%1lld\n"); +#define UCD9000_SMBUS_THROTTLE_US 1000 +static int throttle_delay_us = UCD9000_SMBUS_THROTTLE_US; + +static int ucd9000_debugfs_show_smbus_throttle_delay(void *data, + u64 *val) +{ + struct i2c_client *client = data; + unsigned long ulval; + int rc; + + rc = i2c_smbus_throttle_value(client, &ulval); + if (rc) + return rc; + + *val = ulval; + + return 0; +} + +static int ucd9000_debugfs_store_smbus_throttle_delay(void *data, + u64 val) +{ + struct i2c_client *client = data; + + throttle_delay_us = val; + return i2c_smbus_throttle_client(client, val); +} +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_smbus_throttle_delay, + ucd9000_debugfs_show_smbus_throttle_delay, + ucd9000_debugfs_store_smbus_throttle_delay, "%llu\n"); + static ssize_t ucd9000_debugfs_read_mfr_status(struct file *file, char __user *buf, size_t count, loff_t *ppos) @@ -475,6 +506,8 @@ static int ucd9000_init_debugfs(struct i2c_client *client, scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status"); debugfs_create_file(name, 0444, data->debugfs, client, &ucd9000_debugfs_show_mfr_status_fops); + debugfs_create_file("smbus_throttle_delay", 0664, data->debugfs, client, + &ucd9000_debugfs_smbus_throttle_delay); return 0; } @@ -503,7 +536,9 @@ static int ucd9000_probe(struct i2c_client *client) I2C_FUNC_SMBUS_BLOCK_DATA)) return -ENODEV; - i2c_smbus_throttle_client(client, UCD9000_SMBUS_THROTTLE_US); + ret = i2c_smbus_throttle_client(client, throttle_delay_us); + if (ret) + return ret; ret = i2c_smbus_read_block_data(client, UCD9000_DEVICE_ID, block_buffer); -- 2.27.0
From: Andrew Geissler <geisonator@yahoo.com> This is a workaround requested by bringup team to see if some of the instabilities being seen in the lab clear up. This should not be upstreamed until the hardware team gets some more data on the issue. Signed-off-by: Andrew Geissler <geisonator@yahoo.com> --- drivers/fsi/fsi-master-aspeed.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c index 8606e55c1721..12d36ede6e4b 100644 --- a/drivers/fsi/fsi-master-aspeed.c +++ b/drivers/fsi/fsi-master-aspeed.c @@ -87,7 +87,8 @@ static const u32 fsi_base = 0xa0000000; #define FSI_LINK_ENABLE_SETUP_TIME 10 /* in mS */ /* Run the bus at maximum speed by default */ -#define FSI_DIVISOR_DEFAULT 1 +/* TODO - Run at 100MHz for now to help hw bringup debug */ +#define FSI_DIVISOR_DEFAULT 2 #define FSI_DIVISOR_CABLED 2 static u16 aspeed_fsi_divisor = FSI_DIVISOR_DEFAULT; module_param_named(bus_div,aspeed_fsi_divisor, ushort, 0); -- 2.27.0
From: Andrew Jeffery <andrew@aj.id.au> From extensive testing and tracing it was discovered that the MAX31785 occasionally fails to switch pages despite ACK'ing the PAGE PMBus data write. I suspect this behaviour had been seen on other devices as well, as pmbus_set_page() already read-back the freshly set value and errored out if it wasn't what we requested. In the case of the MAX31785 it was shown that a one-shot retry was enough to get the PAGE write to stick if the inital command failed. To improve robustness, only error out if the one-shot retry also fails to stick. OpenBMC-Staging-Count: 1 Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Signed-off-by: Joel Stanley <joel@jms.id.au> --- drivers/hwmon/pmbus/pmbus_core.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index def8be831456..1a13d6d3f8b5 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -151,25 +151,34 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase) if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL) && data->info->pages > 1 && page != data->currpage) { + int i; + dev_dbg(&client->dev, "Want page %u, %u cached\n", page, data->currpage); - rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); - if (rv < 0) { + for (i = 0; i < 2; i++) { rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); - dev_dbg(&client->dev, - "Failed to set page %u, performed one-shot retry %s: %d\n", - page, rv ? "and failed" : "with success", rv); + if (rv) + continue; + + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE); if (rv < 0) - return rv; - } + continue; - rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE); - if (rv < 0) - return rv; + /* Success, exit loop */ + if (rv == page) + break; + + rv = i2c_smbus_read_byte_data(client, PMBUS_STATUS_CML); + if (rv < 0) + continue; + + if (rv & PB_CML_FAULT_INVALID_DATA) + return -EIO; + } - if (rv != page) + if (i == 2) return -EIO; } data->currpage = page; -- 2.27.0
From: Andrew Jeffery <andrew@aj.id.au> Extensive testing and tracing has shown that the MAX31785 is unreliable in the face of PAGE write commands, ACK'ing the PAGE request but reporting a value of 0 on some subsequent PAGE reads. The trace data suggests that a one-shot retry of the PAGE write is enough to get the requested value to stick. As we configure the device before registering with the PMBus core, centralise PAGE handling inside the driver and implement the one-shot retry semantics there. OpenBMC-Staging-Count: 1 Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Signed-off-by: Joel Stanley <joel@jms.id.au> --- drivers/hwmon/pmbus/max31785.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c index d097f72d4d47..7518fff356f9 100644 --- a/drivers/hwmon/pmbus/max31785.c +++ b/drivers/hwmon/pmbus/max31785.c @@ -362,6 +362,27 @@ static int max31785_write_word_data(struct i2c_client *client, int page, return -ENXIO; } +static int max31785_pmbus_set_page(struct i2c_client *client, int page) +{ + int ret; + int i; + + for (i = 0; i < 2; i++) { + ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); + if (ret < 0) + return ret; + + ret = max31785_i2c_smbus_read_byte_data(client, PMBUS_PAGE); + if (ret < 0) + return ret; + + if (ret == page) + return 0; + } + + return -EIO; +} + /* * Returns negative error codes if an unrecoverable problem is detected, 0 if a * recoverable problem is detected, or a positive value on success. @@ -392,7 +413,7 @@ static int max31785_of_fan_config(struct i2c_client *client, return -ENXIO; } - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); + ret = max31785_pmbus_set_page(client, page); if (ret < 0) return ret; @@ -599,7 +620,7 @@ static int max31785_of_tmp_config(struct i2c_client *client, return -ENXIO; } - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); + ret = max31785_pmbus_set_page(client, page); if (ret < 0) return ret; @@ -700,7 +721,7 @@ static int max31785_configure_dual_tach(struct i2c_client *client, int i; for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) { - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, i); + ret = max31785_pmbus_set_page(client, i); if (ret < 0) return ret; @@ -741,7 +762,7 @@ static int max31785_probe(struct i2c_client *client) *info = max31785_info; - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255); + ret = max31785_pmbus_set_page(client, 255); if (ret < 0) return ret; @@ -785,8 +806,7 @@ static int max31785_probe(struct i2c_client *client) if (!have_fan || fan_configured) continue; - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, - i); + ret = max31785_pmbus_set_page(client, i); if (ret < 0) return ret; -- 2.27.0
From: Andrew Jeffery <andrew@aj.id.au> It has been observed across large fleets of systems that a small subset of those systems occasionally loose control of some number of fans across a BMC reboot (their hwmon fan attributes are missing from sysfs). >From extensive testing and tracing it was discovered that writes enabling a fan in FAN_CONFIG_1_2 failed to stick on the system under test with a frequency of about 1 in 1000 re-binds of the driver. The MAX31785 datasheet recommends in the documentation for MFR_FAN_CONFIG that the asssociated fan(s) be disabled before updating the register. The sequence in question implements this suggestion, and the observed loss-of-fans symptom occurs when the write to re-enable the fan in FAN_CONFIG_1_2 fails to stick. The trace data suggests a one-shot retry is enough to successfully update FAN_CONFIG_1_2. With the workaround, no loss of fans was observed in over 20,000 consecutive rebinds of the driver. OpenBMC-Staging-Count: 1 Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Signed-off-by: Joel Stanley <joel@jms.id.au> --- drivers/hwmon/pmbus/max31785.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c index 7518fff356f9..b37da2ec1ce4 100644 --- a/drivers/hwmon/pmbus/max31785.c +++ b/drivers/hwmon/pmbus/max31785.c @@ -398,6 +398,7 @@ static int max31785_of_fan_config(struct i2c_client *client, u32 page; u32 uval; int ret; + int i; if (!of_device_is_compatible(child, "pmbus-fan")) return 0; @@ -574,10 +575,24 @@ static int max31785_of_fan_config(struct i2c_client *client, if (ret < 0) return ret; - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12, - pb_cfg); - if (ret < 0) - return ret; + for (i = 0; i < 2; i++) { + ret = max31785_i2c_smbus_write_byte_data(client, + PMBUS_FAN_CONFIG_12, + pb_cfg); + if (ret < 0) + continue; + + ret = max31785_i2c_smbus_read_byte_data(client, + PMBUS_FAN_CONFIG_12); + if (ret < 0) + continue; + + if (ret == pb_cfg) + break; + } + + if (i == 2) + return -EIO; /* * Fans are on pages 0 - 5. If the page property of a fan node is -- 2.27.0
Add the necessary Max chip specific fan properties. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 42 ++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts index f87bc5dc8aba..6fd3ddf97a21 100644 --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts @@ -1282,36 +1282,78 @@ fan0: fan@0 { compatible = "pmbus-fan"; reg = <0>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan1: fan@1 { compatible = "pmbus-fan"; reg = <1>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan2: fan@2 { compatible = "pmbus-fan"; reg = <2>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan3: fan@3 { compatible = "pmbus-fan"; reg = <3>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan4: fan@4 { compatible = "pmbus-fan"; reg = <4>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan5: fan@5 { compatible = "pmbus-fan"; reg = <5>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; }; -- 2.27.0
Add the necessary Max chip specific fan properties. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts | 28 ++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts index 01a0b2b03ddd..83fc29309154 100644 --- a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts @@ -2510,24 +2510,52 @@ fan@0 { compatible = "pmbus-fan"; reg = <0>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan@1 { compatible = "pmbus-fan"; reg = <1>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan@2 { compatible = "pmbus-fan"; reg = <2>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; fan@3 { compatible = "pmbus-fan"; reg = <3>; tach-pulses = <2>; + maxim,fan-rotor-input = "tach"; + maxim,fan-pwm-freq = <25000>; + maxim,fan-dual-tach; + maxim,fan-no-watchdog; + maxim,fan-no-fault-ramp; + maxim,fan-ramp = <2>; + maxim,fan-fault-pin-mon; }; }; -- 2.27.0
The fans in the 4U chassis do not have dual tachometers, so remove those properties in the device tree. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- .../boot/dts/aspeed-bmc-ibm-rainier-4u.dts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts index 342546a3c0f5..24283cc3d486 100644 --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts @@ -19,3 +19,27 @@ power-supply@6b { reg = <0x6b>; }; }; + +&fan0 { + /delete-property/ maxim,fan-dual-tach; +}; + +&fan1 { + /delete-property/ maxim,fan-dual-tach; +}; + +&fan2 { + /delete-property/ maxim,fan-dual-tach; +}; + +&fan3 { + /delete-property/ maxim,fan-dual-tach; +}; + +&fan4 { + /delete-property/ maxim,fan-dual-tach; +}; + +&fan5 { + /delete-property/ maxim,fan-dual-tach; +}; -- 2.27.0
The original BMC card must be supported in the device tree, so add new pass 1 device trees for the two relevant Rainier systems. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- arch/arm/boot/dts/Makefile | 2 + .../boot/dts/aspeed-bmc-ibm-rainier-4u-p1.dts | 94 +++++++++++++++++++ .../boot/dts/aspeed-bmc-ibm-rainier-p1.dts | 94 +++++++++++++++++++ 3 files changed, 190 insertions(+) create mode 100644 arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u-p1.dts create mode 100644 arch/arm/boot/dts/aspeed-bmc-ibm-rainier-p1.dts diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index 48d48c85de9e..36b4798e7cd4 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -1402,8 +1402,10 @@ dtb-$(CONFIG_ARCH_ASPEED) += \ aspeed-bmc-facebook-yosemitev2.dtb \ aspeed-bmc-ibm-everest.dtb \ aspeed-bmc-ibm-rainier.dtb \ + aspeed-bmc-ibm-rainier-p1.dtb \ aspeed-bmc-ibm-rainier-1s4u.dtb \ aspeed-bmc-ibm-rainier-4u.dtb \ + aspeed-bmc-ibm-rainier-4u-p1.dtb \ aspeed-bmc-intel-s2600wf.dtb \ aspeed-bmc-inspur-fp5280g2.dtb \ aspeed-bmc-inspur-nf5280m6.dtb \ diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u-p1.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u-p1.dts new file mode 100644 index 000000000000..e35a3efd9d22 --- /dev/null +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u-p1.dts @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// Copyright 2021 IBM Corp. +/dts-v1/; + +#include "aspeed-bmc-ibm-rainier-4u.dts" + +/ { + model = "Rainier 4U Pass 1"; + + aliases { + /delete-property/ i2c20; + /delete-property/ i2c21; + /delete-property/ i2c22; + /delete-property/ i2c23; + /delete-property/ i2c24; + /delete-property/ i2c25; + /delete-property/ i2c26; + /delete-property/ i2c27; + /delete-property/ i2c28; + /delete-property/ i2c29; + /delete-property/ i2c30; + }; +}; + +&i2c4 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; + + eeprom@52 { + compatible = "atmel,24c64"; + reg = <0x52>; + }; +}; + +&i2c5 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; +}; + +&i2c6 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; + + eeprom@52 { + compatible = "atmel,24c64"; + reg = <0x52>; + }; + + eeprom@53 { + compatible = "atmel,24c64"; + reg = <0x53>; + }; +}; + +&i2c11 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; +}; diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-p1.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-p1.dts new file mode 100644 index 000000000000..b3c923f1838b --- /dev/null +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-p1.dts @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// Copyright 2021 IBM Corp. +/dts-v1/; + +#include "aspeed-bmc-ibm-rainier.dts" + +/ { + model = "Rainier 2U Pass 1"; + + aliases { + /delete-property/ i2c20; + /delete-property/ i2c21; + /delete-property/ i2c22; + /delete-property/ i2c23; + /delete-property/ i2c24; + /delete-property/ i2c25; + /delete-property/ i2c26; + /delete-property/ i2c27; + /delete-property/ i2c28; + /delete-property/ i2c29; + /delete-property/ i2c30; + }; +}; + +&i2c4 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; + + eeprom@52 { + compatible = "atmel,24c64"; + reg = <0x52>; + }; +}; + +&i2c5 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; +}; + +&i2c6 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; + + eeprom@52 { + compatible = "atmel,24c64"; + reg = <0x52>; + }; + + eeprom@53 { + compatible = "atmel,24c64"; + reg = <0x53>; + }; +}; + +&i2c11 { + /delete-node/ pca9546@70; + + eeprom@50 { + compatible = "atmel,24c64"; + reg = <0x50>; + }; + + eeprom@51 { + compatible = "atmel,24c64"; + reg = <0x51>; + }; +}; -- 2.27.0
Different commands (with different sizes) require different timeouts, the longest of which can be up to 30 seconds. Adjust the command timeout accordingly. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/fsi/fsi-sbefifo.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c index 84cb965bfed5..8645a092af89 100644 --- a/drivers/fsi/fsi-sbefifo.c +++ b/drivers/fsi/fsi-sbefifo.c @@ -102,12 +102,12 @@ enum sbe_state #define sbefifo_eot_set(sts) (((sts) & SBEFIFO_STS_EOT_MASK) >> SBEFIFO_STS_EOT_SHIFT) /* Reset request timeout in ms */ -#define SBEFIFO_RESET_TIMEOUT 10000 +#define SBEFIFO_RESET_TIMEOUT 30000 /* Timeouts for commands in ms */ -#define SBEFIFO_TIMEOUT_START_CMD 10000 +#define SBEFIFO_TIMEOUT_START_CMD 30000 #define SBEFIFO_TIMEOUT_IN_CMD 1000 -#define SBEFIFO_TIMEOUT_START_RSP 10000 +#define SBEFIFO_TIMEOUT_START_RSP 30000 #define SBEFIFO_TIMEOUT_IN_RSP 1000 /* Other constants */ -- 2.27.0
Use the dynamic branching capability of the dynamic debug subsystem to dump the command and response with the correct OCC device name. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/fsi/fsi-occ.c | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c index 90795325b7d9..b0c9322078a1 100644 --- a/drivers/fsi/fsi-occ.c +++ b/drivers/fsi/fsi-occ.c @@ -21,6 +21,15 @@ #include <linux/uaccess.h> #include <asm/unaligned.h> +#if !defined(CONFIG_DYNAMIC_DEBUG_CORE) +#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) +#if defined(DEBUG) +#define DYNAMIC_DEBUG_BRANCH(descriptor) true +#else /* DEBUG */ +#define DYNAMIC_DEBUG_BRANCH(descriptor) false +#endif /* DEBUG */ +#endif /* CONFIG_DYNAMIC_DEBUG_CORE */ + #define OCC_SRAM_BYTES 4096 #define OCC_CMD_DATA_BYTES 4090 #define OCC_RESP_DATA_BYTES 4089 @@ -359,6 +368,20 @@ static int occ_putsram(struct occ *occ, const void *data, ssize_t len, byte_buf[len - 2] = checksum >> 8; byte_buf[len - 1] = checksum & 0xff; + { + DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_cmd, "OCC command"); + + if (DYNAMIC_DEBUG_BRANCH(ddm_occ_cmd)) { + char prefix[64]; + + snprintf(prefix, sizeof(prefix), "%s %s: cmd ", + dev_driver_string(occ->dev), + dev_name(occ->dev)); + print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_OFFSET, + 16, 4, byte_buf, len, false); + } + } + rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len); if (rc) goto free; @@ -556,6 +579,27 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len, } *resp_len = resp_data_length + 7; + + { + DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_rsp, + "OCC response"); + DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_full_rsp, + "OCC full response"); + + if (DYNAMIC_DEBUG_BRANCH(ddm_occ_full_rsp) || + DYNAMIC_DEBUG_BRANCH(ddm_occ_rsp)) { + char prefix[64]; + size_t l = DYNAMIC_DEBUG_BRANCH(ddm_occ_full_rsp) ? + *resp_len : 16; + + snprintf(prefix, sizeof(prefix), "%s %s: rsp ", + dev_driver_string(occ->dev), + dev_name(occ->dev)); + print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_OFFSET, + 16, 4, resp, l, false); + } + } + rc = occ_verify_checksum(occ, resp, resp_data_length); done: -- 2.27.0
Some SBE operations have extremely large responses and can require several minutes to process the response. During this time, the device lock must be held. If another process attempts an operation, it will wait for the mutex for longer than the kernel hung task watchdog allows. Therefore, use the interruptible function to lock the mutex. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/fsi/fsi-sbefifo.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c index 8645a092af89..060a790989f9 100644 --- a/drivers/fsi/fsi-sbefifo.c +++ b/drivers/fsi/fsi-sbefifo.c @@ -740,7 +740,9 @@ int sbefifo_submit(struct device *dev, const __be32 *command, size_t cmd_len, iov_iter_kvec(&resp_iter, WRITE, &resp_iov, 1, rbytes); /* Perform the command */ - mutex_lock(&sbefifo->lock); + rc = mutex_lock_interruptible(&sbefifo->lock); + if (rc) + return rc; rc = __sbefifo_submit(sbefifo, command, cmd_len, &resp_iter); mutex_unlock(&sbefifo->lock); @@ -820,7 +822,9 @@ static ssize_t sbefifo_user_read(struct file *file, char __user *buf, iov_iter_init(&resp_iter, WRITE, &resp_iov, 1, len); /* Perform the command */ - mutex_lock(&sbefifo->lock); + rc = mutex_lock_interruptible(&sbefifo->lock); + if (rc) + goto bail; rc = __sbefifo_submit(sbefifo, user->pending_cmd, cmd_len, &resp_iter); mutex_unlock(&sbefifo->lock); if (rc < 0) @@ -875,7 +879,9 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf, user->pending_len = 0; /* Trigger reset request */ - mutex_lock(&sbefifo->lock); + rc = mutex_lock_interruptible(&sbefifo->lock); + if (rc) + goto bail; rc = sbefifo_request_reset(user->sbefifo); mutex_unlock(&sbefifo->lock); if (rc == 0) -- 2.27.0
On 8/11/21 10:42 AM, Eddie James wrote: > From: Andrew Jeffery <andrew@aj.id.au> > > Extensive testing and tracing has shown that the MAX31785 is unreliable > in the face of PAGE write commands, ACK'ing the PAGE request but > reporting a value of 0 on some subsequent PAGE reads. The trace data > suggests that a one-shot retry of the PAGE write is enough to get the > requested value to stick. > > As we configure the device before registering with the PMBus core, > centralise PAGE handling inside the driver and implement the one-shot > retry semantics there. > > OpenBMC-Staging-Count: 1 > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Signed-off-by: Joel Stanley <joel@jms.id.au> Reviewed-by: Matthew Barth <msbarth@linux.ibm.com> > --- > drivers/hwmon/pmbus/max31785.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c > index d097f72d4d47..7518fff356f9 100644 > --- a/drivers/hwmon/pmbus/max31785.c > +++ b/drivers/hwmon/pmbus/max31785.c > @@ -362,6 +362,27 @@ static int max31785_write_word_data(struct i2c_client *client, int page, > return -ENXIO; > } > > +static int max31785_pmbus_set_page(struct i2c_client *client, int page) > +{ > + int ret; > + int i; > + > + for (i = 0; i < 2; i++) { > + ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > + if (ret < 0) > + return ret; > + > + ret = max31785_i2c_smbus_read_byte_data(client, PMBUS_PAGE); > + if (ret < 0) > + return ret; > + > + if (ret == page) > + return 0; > + } > + > + return -EIO; > +} > + > /* > * Returns negative error codes if an unrecoverable problem is detected, 0 if a > * recoverable problem is detected, or a positive value on success. > @@ -392,7 +413,7 @@ static int max31785_of_fan_config(struct i2c_client *client, > return -ENXIO; > } > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > + ret = max31785_pmbus_set_page(client, page); > if (ret < 0) > return ret; > > @@ -599,7 +620,7 @@ static int max31785_of_tmp_config(struct i2c_client *client, > return -ENXIO; > } > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > + ret = max31785_pmbus_set_page(client, page); > if (ret < 0) > return ret; > > @@ -700,7 +721,7 @@ static int max31785_configure_dual_tach(struct i2c_client *client, > int i; > > for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) { > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, i); > + ret = max31785_pmbus_set_page(client, i); > if (ret < 0) > return ret; > > @@ -741,7 +762,7 @@ static int max31785_probe(struct i2c_client *client) > > *info = max31785_info; > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255); > + ret = max31785_pmbus_set_page(client, 255); > if (ret < 0) > return ret; > > @@ -785,8 +806,7 @@ static int max31785_probe(struct i2c_client *client) > if (!have_fan || fan_configured) > continue; > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, > - i); > + ret = max31785_pmbus_set_page(client, i); > if (ret < 0) > return ret; >
On 8/11/21 10:42 AM, Eddie James wrote: > From: Andrew Jeffery <andrew@aj.id.au> > > It has been observed across large fleets of systems that a small subset > of those systems occasionally loose control of some number of fans > across a BMC reboot (their hwmon fan attributes are missing from sysfs). > > >From extensive testing and tracing it was discovered that writes > enabling a fan in FAN_CONFIG_1_2 failed to stick on the system under > test with a frequency of about 1 in 1000 re-binds of the driver. > > The MAX31785 datasheet recommends in the documentation for > MFR_FAN_CONFIG that the asssociated fan(s) be disabled before updating > the register. The sequence in question implements this suggestion, and > the observed loss-of-fans symptom occurs when the write to re-enable the > fan in FAN_CONFIG_1_2 fails to stick. > > The trace data suggests a one-shot retry is enough to successfully > update FAN_CONFIG_1_2. With the workaround, no loss of fans was observed > in over 20,000 consecutive rebinds of the driver. > > OpenBMC-Staging-Count: 1 > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Signed-off-by: Joel Stanley <joel@jms.id.au> Reviewed-by: Matthew Barth <msbarth@linux.ibm.com> > --- > drivers/hwmon/pmbus/max31785.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c > index 7518fff356f9..b37da2ec1ce4 100644 > --- a/drivers/hwmon/pmbus/max31785.c > +++ b/drivers/hwmon/pmbus/max31785.c > @@ -398,6 +398,7 @@ static int max31785_of_fan_config(struct i2c_client *client, > u32 page; > u32 uval; > int ret; > + int i; > > if (!of_device_is_compatible(child, "pmbus-fan")) > return 0; > @@ -574,10 +575,24 @@ static int max31785_of_fan_config(struct i2c_client *client, > if (ret < 0) > return ret; > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12, > - pb_cfg); > - if (ret < 0) > - return ret; > + for (i = 0; i < 2; i++) { > + ret = max31785_i2c_smbus_write_byte_data(client, > + PMBUS_FAN_CONFIG_12, > + pb_cfg); > + if (ret < 0) > + continue; > + > + ret = max31785_i2c_smbus_read_byte_data(client, > + PMBUS_FAN_CONFIG_12); > + if (ret < 0) > + continue; > + > + if (ret == pb_cfg) > + break; > + } > + > + if (i == 2) > + return -EIO; > > /* > * Fans are on pages 0 - 5. If the page property of a fan node is
On 8/11/21 10:42 AM, Eddie James wrote: > Add the necessary Max chip specific fan properties. > > Signed-off-by: Eddie James <eajames@linux.ibm.com> Reviewed-by: Matthew Barth <msbarth@linux.ibm.com> > --- > arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 42 ++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts > index f87bc5dc8aba..6fd3ddf97a21 100644 > --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts > +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts > @@ -1282,36 +1282,78 @@ fan0: fan@0 { > compatible = "pmbus-fan"; > reg = <0>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan1: fan@1 { > compatible = "pmbus-fan"; > reg = <1>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan2: fan@2 { > compatible = "pmbus-fan"; > reg = <2>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan3: fan@3 { > compatible = "pmbus-fan"; > reg = <3>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan4: fan@4 { > compatible = "pmbus-fan"; > reg = <4>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan5: fan@5 { > compatible = "pmbus-fan"; > reg = <5>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > }; >
On 8/11/21 10:42 AM, Eddie James wrote: > Add the necessary Max chip specific fan properties. > > Signed-off-by: Eddie James <eajames@linux.ibm.com> Reviewed-by: Matthew Barth <msbarth@linux.ibm.com> > --- > arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts | 28 ++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts > index 01a0b2b03ddd..83fc29309154 100644 > --- a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts > +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts > @@ -2510,24 +2510,52 @@ fan@0 { > compatible = "pmbus-fan"; > reg = <0>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan@1 { > compatible = "pmbus-fan"; > reg = <1>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan@2 { > compatible = "pmbus-fan"; > reg = <2>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > > fan@3 { > compatible = "pmbus-fan"; > reg = <3>; > tach-pulses = <2>; > + maxim,fan-rotor-input = "tach"; > + maxim,fan-pwm-freq = <25000>; > + maxim,fan-dual-tach; > + maxim,fan-no-watchdog; > + maxim,fan-no-fault-ramp; > + maxim,fan-ramp = <2>; > + maxim,fan-fault-pin-mon; > }; > }; >
On 8/11/21 10:42 AM, Eddie James wrote: > The fans in the 4U chassis do not have dual tachometers, so remove those > properties in the device tree. > > Signed-off-by: Eddie James <eajames@linux.ibm.com> Reviewed-by: Matthew Barth <msbarth@linux.ibm.com> > --- > .../boot/dts/aspeed-bmc-ibm-rainier-4u.dts | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts > index 342546a3c0f5..24283cc3d486 100644 > --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts > +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier-4u.dts > @@ -19,3 +19,27 @@ power-supply@6b { > reg = <0x6b>; > }; > }; > + > +&fan0 { > + /delete-property/ maxim,fan-dual-tach; > +}; > + > +&fan1 { > + /delete-property/ maxim,fan-dual-tach; > +}; > + > +&fan2 { > + /delete-property/ maxim,fan-dual-tach; > +}; > + > +&fan3 { > + /delete-property/ maxim,fan-dual-tach; > +}; > + > +&fan4 { > + /delete-property/ maxim,fan-dual-tach; > +}; > + > +&fan5 { > + /delete-property/ maxim,fan-dual-tach; > +};