| Message ID | 20190909222634.26146-3-jae.hyun.yoo@linux.intel.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | i2c: aspeed: Add H/W timeout support | expand |
On 9/9/19 3:26 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@linux.intel.com> The patch looks good to me. Thank you Jae for making the changes. Reviewed-by: Tao Ren <taoren@fb.com> Cheers, Tao > --- > Changes since v2: > - Moved 'aspeed,hw-timeout-ms' property parsing logic to make it can > be done before aspeed_i2c_init_clk. > > Changes since v1: > - Made it use bus auto recovery feature so that bus can recover itself > automatically. > > drivers/i2c/busses/i2c-aspeed.c | 82 ++++++++++++++++++++++++++++++--- > 1 file changed, 76 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 89317929bee4..c66eca9a00f4 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -55,6 +55,7 @@ > /* Device Register Definition */ > /* 0x00 : I2CD Function Control Register */ > #define ASPEED_I2CD_BUFFER_PAGE_SEL_MASK GENMASK(22, 20) > +#define ASPEED_I2CD_BUS_AUTO_RECOVERY_EN BIT(17) > #define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15) > #define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8) > #define ASPEED_I2CD_M_SDA_DRIVE_1T_EN BIT(7) > @@ -70,10 +71,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 +86,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 +102,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 +185,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 +286,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 +304,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 +628,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 +659,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 +1220,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 +1229,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; > } > @@ -1218,6 +1283,11 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus, > /* Disable everything. */ > writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG); > > + device_property_read_u32(&pdev->dev, "aspeed,hw-timeout-ms", > + &bus->hw_timeout_ms); > + if (bus->hw_timeout_ms) > + fun_ctrl_reg |= ASPEED_I2CD_BUS_AUTO_RECOVERY_EN; > + > ret = aspeed_i2c_init_clk(bus); > if (ret < 0) > return ret; >
On 9/9/2019 4:40 PM, Tao Ren wrote: > On 9/9/19 3:26 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@linux.intel.com> > > The patch looks good to me. Thank you Jae for making the changes. > > Reviewed-by: Tao Ren <taoren@fb.com> Hi Tao, Thanks a lot for your code review and your help on testing it. I'll submit it to linux upstream as well after completing other backlog I2C patches' upstreaming. Cheers, Jae
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 89317929bee4..c66eca9a00f4 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -55,6 +55,7 @@ /* Device Register Definition */ /* 0x00 : I2CD Function Control Register */ #define ASPEED_I2CD_BUFFER_PAGE_SEL_MASK GENMASK(22, 20) +#define ASPEED_I2CD_BUS_AUTO_RECOVERY_EN BIT(17) #define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15) #define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8) #define ASPEED_I2CD_M_SDA_DRIVE_1T_EN BIT(7) @@ -70,10 +71,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 +86,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 +102,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 +185,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 +286,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 +304,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 +628,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 +659,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 +1220,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 +1229,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; } @@ -1218,6 +1283,11 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus, /* Disable everything. */ writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG); + device_property_read_u32(&pdev->dev, "aspeed,hw-timeout-ms", + &bus->hw_timeout_ms); + if (bus->hw_timeout_ms) + fun_ctrl_reg |= ASPEED_I2CD_BUS_AUTO_RECOVERY_EN; + ret = aspeed_i2c_init_clk(bus); if (ret < 0) return ret;
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@linux.intel.com> --- Changes since v2: - Moved 'aspeed,hw-timeout-ms' property parsing logic to make it can be done before aspeed_i2c_init_clk. Changes since v1: - Made it use bus auto recovery feature so that bus can recover itself automatically. drivers/i2c/busses/i2c-aspeed.c | 82 ++++++++++++++++++++++++++++++--- 1 file changed, 76 insertions(+), 6 deletions(-)