[PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
Tao Ren
taoren at fb.com
Fri Sep 6 08:28:02 AEST 2019
Hi Jae,
On 9/4/19 1:07 PM, Jae Hyun Yoo wrote:
> In case of multi-master environment, if a peer master incorrectly handles
> a bus in the middle of a transaction, I2C hardware hangs in slave state
> and it can't escape from the slave state, so this commit adds slave
> inactive timeout support to recover the bus in the case.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com>
I have a generic question on the patch: is it good enough to just enable slave_inactive_timeout and acknowledge the interrupt when it happens? Or do we need to reset the i2c controller to bring it out of slave state?
I'm asking this because I hit an issue on my Minipack BMC where the slave_inactive_timeout interrupt was generated once every ~0.1 seconds (I set aspeed,hw-timeout-ms to 1000), and a few seconds later my BMC was rebooted (I guess watchdog timed out). Given the issue happened only once, I don't have chance to collect more information (such as why the repetitive interrupt was generated, why BMC rebooted, and etc.). Will share more if I met the problem again.
Cheers,
Tao
> ---
> drivers/i2c/busses/i2c-aspeed.c | 79 ++++++++++++++++++++++++++++++---
> 1 file changed, 73 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 89317929bee4..92e1a249b393 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -70,10 +70,14 @@
> #define ASPEED_I2CD_TIME_SCL_HIGH_MASK GENMASK(19, 16)
> #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT 12
> #define ASPEED_I2CD_TIME_SCL_LOW_MASK GENMASK(15, 12)
> +#define ASPEED_I2CD_TIME_TIMEOUT_BASE_DIVISOR_SHIFT 8
> +#define ASPEED_I2CD_TIME_TIMEOUT_BASE_DIVISOR_MASK GENMASK(9, 8)
> #define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK GENMASK(3, 0)
> #define ASPEED_I2CD_TIME_SCL_REG_MAX GENMASK(3, 0)
> +
> /* 0x08 : I2CD Clock and AC Timing Control Register #2 */
> -#define ASPEED_NO_TIMEOUT_CTRL 0
> +#define ASPEED_I2CD_TIMEOUT_CYCLES_SHIFT 0
> +#define ASPEED_I2CD_TIMEOUT_CYCLES_MASK GENMASK(4, 0)
>
> /* 0x0c : I2CD Interrupt Control Register &
> * 0x10 : I2CD Interrupt Status Register
> @@ -81,6 +85,7 @@
> * These share bit definitions, so use the same values for the enable &
> * status bits.
> */
> +#define ASPEED_I2CD_INTR_SLAVE_INACTIVE_TIMEOUT BIT(15)
> #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
> #define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
> #define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
> @@ -96,8 +101,11 @@
> ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> ASPEED_I2CD_INTR_ABNORMAL | \
> ASPEED_I2CD_INTR_ARBIT_LOSS)
> +#define ASPEED_I2CD_INTR_SLAVE_ERRORS \
> + ASPEED_I2CD_INTR_SLAVE_INACTIVE_TIMEOUT
> #define ASPEED_I2CD_INTR_ALL \
> - (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> + (ASPEED_I2CD_INTR_SLAVE_INACTIVE_TIMEOUT | \
> + ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
> ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> ASPEED_I2CD_INTR_ABNORMAL | \
> @@ -176,6 +184,7 @@ struct aspeed_i2c_bus {
> u32 divisor);
> unsigned long parent_clk_frequency;
> u32 bus_frequency;
> + u32 hw_timeout_ms;
> /* Transaction state. */
> enum aspeed_i2c_master_state master_state;
> struct i2c_msg *msgs;
> @@ -276,6 +285,14 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> }
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int aspeed_i2c_check_slave_error(u32 irq_status)
> +{
> + if (irq_status & ASPEED_I2CD_INTR_SLAVE_INACTIVE_TIMEOUT)
> + return -EIO;
> +
> + return 0;
> +}
> +
> static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> u32 command, irq_handled = 0;
> @@ -286,6 +303,14 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> if (!slave)
> return 0;
>
> + if (aspeed_i2c_check_slave_error(irq_status)) {
> + dev_dbg(bus->dev, "received slave error interrupt: 0x%08x\n",
> + irq_status);
> + irq_handled |= (irq_status & ASPEED_I2CD_INTR_SLAVE_ERRORS);
> + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
> + return irq_handled;
> + }
> +
> command = readl(bus->base + ASPEED_I2C_CMD_REG);
>
> /* Slave was requested, restart state machine. */
> @@ -602,7 +627,7 @@ static void aspeed_i2c_next_msg_or_stop(struct aspeed_i2c_bus *bus)
> }
> }
>
> -static int aspeed_i2c_is_irq_error(u32 irq_status)
> +static int aspeed_i2c_check_master_error(u32 irq_status)
> {
> if (irq_status & ASPEED_I2CD_INTR_ARBIT_LOSS)
> return -EAGAIN;
> @@ -633,9 +658,9 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> * should clear the command queue effectively taking us back to the
> * INACTIVE state.
> */
> - ret = aspeed_i2c_is_irq_error(irq_status);
> + ret = aspeed_i2c_check_master_error(irq_status);
> if (ret) {
> - dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> + dev_dbg(bus->dev, "received master error interrupt: 0x%08x\n",
> irq_status);
> irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
> if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> @@ -1194,6 +1219,7 @@ static u32 aspeed_i2c_25xx_get_clk_reg_val(struct device *dev, u32 divisor)
> /* precondition: bus.lock has been acquired. */
> static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
> {
> + u32 timeout_base_divisor, timeout_tick_us, timeout_cycles;
> u32 divisor, clk_reg_val;
>
> divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> @@ -1202,8 +1228,46 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
> ASPEED_I2CD_TIME_THDSTA_MASK |
> ASPEED_I2CD_TIME_TACST_MASK);
> clk_reg_val |= bus->get_clk_reg_val(bus->dev, divisor);
> +
> + if (bus->hw_timeout_ms) {
> + u8 div_max = ASPEED_I2CD_TIME_TIMEOUT_BASE_DIVISOR_MASK >>
> + ASPEED_I2CD_TIME_TIMEOUT_BASE_DIVISOR_SHIFT;
> + u8 cycles_max = ASPEED_I2CD_TIMEOUT_CYCLES_MASK >>
> + ASPEED_I2CD_TIMEOUT_CYCLES_SHIFT;
> +
> + timeout_base_divisor = 0;
> +
> + do {
> + timeout_tick_us = 1000 * (16384 <<
> + (timeout_base_divisor << 1)) /
> + (bus->parent_clk_frequency / 1000);
> +
> + if (timeout_base_divisor == div_max ||
> + timeout_tick_us * ASPEED_I2CD_TIMEOUT_CYCLES_MASK >=
> + bus->hw_timeout_ms * 1000)
> + break;
> + } while (timeout_base_divisor++ < div_max);
> +
> + if (timeout_tick_us) {
> + timeout_cycles = DIV_ROUND_UP(bus->hw_timeout_ms * 1000,
> + timeout_tick_us);
> + if (timeout_cycles == 0)
> + timeout_cycles = 1;
> + else if (timeout_cycles > cycles_max)
> + timeout_cycles = cycles_max;
> + } else {
> + timeout_cycles = 0;
> + }
> + } else {
> + timeout_base_divisor = 0;
> + timeout_cycles = 0;
> + }
> +
> + clk_reg_val |= FIELD_PREP(ASPEED_I2CD_TIME_TIMEOUT_BASE_DIVISOR_MASK,
> + timeout_base_divisor);
> +
> writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
> - writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
> + writel(timeout_cycles, bus->base + ASPEED_I2C_AC_TIMING_REG2);
>
> return 0;
> }
> @@ -1404,6 +1468,9 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> }
> }
>
> + device_property_read_u32(&pdev->dev, "aspeed,hw-timeout-ms",
> + &bus->hw_timeout_ms);
> +
> /* Initialize the I2C adapter */
> spin_lock_init(&bus->lock);
> init_completion(&bus->cmd_complete);
>
More information about the openbmc
mailing list