[PATCH v3 dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support

Tao Ren taoren at fb.com
Tue Sep 10 09:40:33 AEST 2019


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 at linux.intel.com>

The patch looks good to me. Thank you Jae for making the changes.

Reviewed-by: Tao Ren <taoren at 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;
> 


More information about the openbmc mailing list