* [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
@ 2019-09-04 20:07 Jae Hyun Yoo
2019-09-04 20:07 ` [PATCH dev-5.2 1/2] dt-bindings: i2c: aspeed: add hardware " Jae Hyun Yoo
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-04 20:07 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Ryan Chen, Tao Ren
Cc: openbmc, Jae Hyun Yoo
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.
By applying this change, SDA data-low and SCL clock-low timeout feature
also could be enabled which was disabled previously.
Jae Hyun Yoo (2):
dt-bindings: i2c: aspeed: add hardware timeout support
i2c: aspeed: add slave inactive timeout support
.../devicetree/bindings/i2c/i2c-aspeed.txt | 2 +
drivers/i2c/busses/i2c-aspeed.c | 79 +++++++++++++++++--
2 files changed, 75 insertions(+), 6 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH dev-5.2 1/2] dt-bindings: i2c: aspeed: add hardware timeout support
2019-09-04 20:07 [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support Jae Hyun Yoo
@ 2019-09-04 20:07 ` Jae Hyun Yoo
2019-09-04 20:07 ` [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive " Jae Hyun Yoo
2019-09-04 22:54 ` [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W " Joel Stanley
2 siblings, 0 replies; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-04 20:07 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Ryan Chen, Tao Ren
Cc: openbmc, Jae Hyun Yoo
Append a binding to support hardware timeout feature.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
Documentation/devicetree/bindings/i2c/i2c-aspeed.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
index e5b46885c15e..71de956ffa4f 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -29,6 +29,8 @@ Optional Properties:
DMA mode instead of PIO or FIFO respectively, I2C
can't use DMA mode. IF both DMA and buffer modes are
enabled, DMA mode will be selected.
+- aspeed,hw-timeout-ms : Hardware timeout in milliseconds. If it's not
+ specified, the H/W timeout feature will be disabled.
Example:
--
2.23.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-04 20:07 [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support Jae Hyun Yoo
2019-09-04 20:07 ` [PATCH dev-5.2 1/2] dt-bindings: i2c: aspeed: add hardware " Jae Hyun Yoo
@ 2019-09-04 20:07 ` Jae Hyun Yoo
2019-09-04 22:37 ` Tao Ren
2019-09-05 22:28 ` Tao Ren
2019-09-04 22:54 ` [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W " Joel Stanley
2 siblings, 2 replies; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-04 20:07 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Ryan Chen, Tao Ren
Cc: openbmc, Jae Hyun Yoo
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>
---
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);
--
2.23.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-04 20:07 ` [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive " Jae Hyun Yoo
@ 2019-09-04 22:37 ` Tao Ren
2019-09-05 22:28 ` Tao Ren
1 sibling, 0 replies; 23+ messages in thread
From: Tao Ren @ 2019-09-04 22:37 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, Ryan Chen
Cc: openbmc
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@linux.intel.com>
Thank you Jae for the patch. Let me play with it and will update back with my results later this week.
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);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-04 20:07 [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support Jae Hyun Yoo
2019-09-04 20:07 ` [PATCH dev-5.2 1/2] dt-bindings: i2c: aspeed: add hardware " Jae Hyun Yoo
2019-09-04 20:07 ` [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive " Jae Hyun Yoo
@ 2019-09-04 22:54 ` Joel Stanley
2019-09-04 23:01 ` Jae Hyun Yoo
2019-09-04 23:50 ` Brendan Higgins
2 siblings, 2 replies; 23+ messages in thread
From: Joel Stanley @ 2019-09-04 22:54 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Brendan Higgins, Benjamin Herrenschmidt, Andrew Jeffery,
Ryan Chen, Tao Ren, OpenBMC Maillist
Hi Jae,
On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
>
> By applying this change, SDA data-low and SCL clock-low timeout feature
> also could be enabled which was disabled previously.
Please consider sending your RFC patches to the upstream list. You
have a big backlog of patches now.
Cheers,
Joel
>
> Jae Hyun Yoo (2):
> dt-bindings: i2c: aspeed: add hardware timeout support
> i2c: aspeed: add slave inactive timeout support
>
> .../devicetree/bindings/i2c/i2c-aspeed.txt | 2 +
> drivers/i2c/busses/i2c-aspeed.c | 79 +++++++++++++++++--
> 2 files changed, 75 insertions(+), 6 deletions(-)
>
> --
> 2.23.0
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-04 22:54 ` [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W " Joel Stanley
@ 2019-09-04 23:01 ` Jae Hyun Yoo
2019-09-04 23:12 ` Andrew Jeffery
2019-09-04 23:50 ` Brendan Higgins
1 sibling, 1 reply; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-04 23:01 UTC (permalink / raw)
To: Joel Stanley
Cc: Ryan Chen, Andrew Jeffery, OpenBMC Maillist, Brendan Higgins, Tao Ren
Hi Joel,
On 9/4/2019 3:54 PM, Joel Stanley wrote:
> Hi Jae,
>
> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
>>
>> By applying this change, SDA data-low and SCL clock-low timeout feature
>> also could be enabled which was disabled previously.
>
> Please consider sending your RFC patches to the upstream list. You
> have a big backlog of patches now.
Thanks for the reminding. I can't send the RFC patches yet because QEMU
H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
Thanks,
Jae
>
> Cheers,
>
> Joel
>
>>
>> Jae Hyun Yoo (2):
>> dt-bindings: i2c: aspeed: add hardware timeout support
>> i2c: aspeed: add slave inactive timeout support
>>
>> .../devicetree/bindings/i2c/i2c-aspeed.txt | 2 +
>> drivers/i2c/busses/i2c-aspeed.c | 79 +++++++++++++++++--
>> 2 files changed, 75 insertions(+), 6 deletions(-)
>>
>> --
>> 2.23.0
>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-04 23:01 ` Jae Hyun Yoo
@ 2019-09-04 23:12 ` Andrew Jeffery
2019-09-04 23:40 ` Jae Hyun Yoo
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Jeffery @ 2019-09-04 23:12 UTC (permalink / raw)
To: Jae Hyun Yoo, Joel Stanley
Cc: Ryan Chen, OpenBMC Maillist, Brendan Higgins, Tao Ren
On Thu, 5 Sep 2019, at 08:31, Jae Hyun Yoo wrote:
> Hi Joel,
>
> On 9/4/2019 3:54 PM, Joel Stanley wrote:
> > Hi Jae,
> >
> > On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
> >>
> >> By applying this change, SDA data-low and SCL clock-low timeout feature
> >> also could be enabled which was disabled previously.
> >
> > Please consider sending your RFC patches to the upstream list. You
> > have a big backlog of patches now.
>
> Thanks for the reminding. I can't send the RFC patches yet because QEMU
> H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
QEMU shouldn't be preventing you from sending patches upstream, rather
it prevents us from enabling the buffer mode support in the OpenBMC
kernel tree. You should be sending all patches upstream as early as possible.
Cheers,
Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-04 23:12 ` Andrew Jeffery
@ 2019-09-04 23:40 ` Jae Hyun Yoo
2019-09-05 0:10 ` Andrew Jeffery
0 siblings, 1 reply; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-04 23:40 UTC (permalink / raw)
To: Andrew Jeffery, Joel Stanley
Cc: Brendan Higgins, OpenBMC Maillist, Ryan Chen, Tao Ren
Hi Andrew,
On 9/4/2019 4:12 PM, Andrew Jeffery wrote:
> On Thu, 5 Sep 2019, at 08:31, Jae Hyun Yoo wrote:
>> Hi Joel,
>>
>> On 9/4/2019 3:54 PM, Joel Stanley wrote:
>>> Hi Jae,
>>>
>>> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
>>>>
>>>> By applying this change, SDA data-low and SCL clock-low timeout feature
>>>> also could be enabled which was disabled previously.
>>>
>>> Please consider sending your RFC patches to the upstream list. You
>>> have a big backlog of patches now.
>>
>> Thanks for the reminding. I can't send the RFC patches yet because QEMU
>> H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
>
> QEMU shouldn't be preventing you from sending patches upstream, rather
> it prevents us from enabling the buffer mode support in the OpenBMC
> kernel tree. You should be sending all patches upstream as early as possible.
I met a QEMU issue when I was upstreaming a patch set last year:
https://lists.ozlabs.org/pipermail/linux-aspeed/2018-September/000750.html
If OpenBMC community accepts the QEMU issue, I can submit the RFC
patches to upstream. Will submit the patch set soon to linux tree.
Thanks,
Jae
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-04 22:54 ` [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W " Joel Stanley
2019-09-04 23:01 ` Jae Hyun Yoo
@ 2019-09-04 23:50 ` Brendan Higgins
2019-09-05 0:56 ` Jae Hyun Yoo
1 sibling, 1 reply; 23+ messages in thread
From: Brendan Higgins @ 2019-09-04 23:50 UTC (permalink / raw)
To: Joel Stanley
Cc: Jae Hyun Yoo, Benjamin Herrenschmidt, Andrew Jeffery, Ryan Chen,
Tao Ren, OpenBMC Maillist
On Wed, Sep 4, 2019 at 3:55 PM Joel Stanley <joel@jms.id.au> wrote:
>
> Hi Jae,
>
> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
> >
> > By applying this change, SDA data-low and SCL clock-low timeout feature
> > also could be enabled which was disabled previously.
>
> Please consider sending your RFC patches to the upstream list. You
> have a big backlog of patches now.
Joel, thanks for keeping track of this!
Sorry, for not keeping up with my emails, I don't think I have time to
continue to maintain this.
However, I don't want to hand this off in a bad state. I will try to
get up to date on all the emails in the coming weeks.
Jae, can you start sending things upstream as Joel suggested?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-04 23:40 ` Jae Hyun Yoo
@ 2019-09-05 0:10 ` Andrew Jeffery
2019-09-05 0:54 ` Jae Hyun Yoo
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Jeffery @ 2019-09-05 0:10 UTC (permalink / raw)
To: Jae Hyun Yoo, Joel Stanley
Cc: Brendan Higgins, OpenBMC Maillist, Ryan Chen, Tao Ren
On Thu, 5 Sep 2019, at 09:10, Jae Hyun Yoo wrote:
> Hi Andrew,
>
> On 9/4/2019 4:12 PM, Andrew Jeffery wrote:
> > On Thu, 5 Sep 2019, at 08:31, Jae Hyun Yoo wrote:
> >> Hi Joel,
> >>
> >> On 9/4/2019 3:54 PM, Joel Stanley wrote:
> >>> Hi Jae,
> >>>
> >>> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
> >>>>
> >>>> By applying this change, SDA data-low and SCL clock-low timeout feature
> >>>> also could be enabled which was disabled previously.
> >>>
> >>> Please consider sending your RFC patches to the upstream list. You
> >>> have a big backlog of patches now.
> >>
> >> Thanks for the reminding. I can't send the RFC patches yet because QEMU
> >> H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
> >
> > QEMU shouldn't be preventing you from sending patches upstream, rather
> > it prevents us from enabling the buffer mode support in the OpenBMC
> > kernel tree. You should be sending all patches upstream as early as possible.
>
> I met a QEMU issue when I was upstreaming a patch set last year:
> https://lists.ozlabs.org/pipermail/linux-aspeed/2018-September/000750.html
>
> If OpenBMC community accepts the QEMU issue, I can submit the RFC
> patches to upstream. Will submit the patch set soon to linux tree.
Ah, didn't realise it was Guenter that ran into it. We have some changes[1] in
Cedric's aspeed-4.2 qemu tree - do you mind testing it out? If those patches
resolve the issue Maybe we could point Guenter at that tree, though really we
should get the fixes upstream so this isn't an issue.
[1] https://github.com/legoater/qemu/compare/59dda66ab756e52e6a9c1ef89660d30b3769f63c...aspeed-4.2
Cheers,
Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-05 0:10 ` Andrew Jeffery
@ 2019-09-05 0:54 ` Jae Hyun Yoo
2019-09-11 18:56 ` Jae Hyun Yoo
0 siblings, 1 reply; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-05 0:54 UTC (permalink / raw)
To: Andrew Jeffery, Joel Stanley
Cc: Brendan Higgins, OpenBMC Maillist, Ryan Chen, Tao Ren
On 9/4/2019 5:10 PM, Andrew Jeffery wrote:
>
>
> On Thu, 5 Sep 2019, at 09:10, Jae Hyun Yoo wrote:
>> Hi Andrew,
>>
>> On 9/4/2019 4:12 PM, Andrew Jeffery wrote:
>>> On Thu, 5 Sep 2019, at 08:31, Jae Hyun Yoo wrote:
>>>> Hi Joel,
>>>>
>>>> On 9/4/2019 3:54 PM, Joel Stanley wrote:
>>>>> Hi Jae,
>>>>>
>>>>> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
>>>>>>
>>>>>> By applying this change, SDA data-low and SCL clock-low timeout feature
>>>>>> also could be enabled which was disabled previously.
>>>>>
>>>>> Please consider sending your RFC patches to the upstream list. You
>>>>> have a big backlog of patches now.
>>>>
>>>> Thanks for the reminding. I can't send the RFC patches yet because QEMU
>>>> H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
>>>
>>> QEMU shouldn't be preventing you from sending patches upstream, rather
>>> it prevents us from enabling the buffer mode support in the OpenBMC
>>> kernel tree. You should be sending all patches upstream as early as possible.
>>
>> I met a QEMU issue when I was upstreaming a patch set last year:
>> https://lists.ozlabs.org/pipermail/linux-aspeed/2018-September/000750.html
>>
>> If OpenBMC community accepts the QEMU issue, I can submit the RFC
>> patches to upstream. Will submit the patch set soon to linux tree.
>
> Ah, didn't realise it was Guenter that ran into it. We have some changes[1] in
> Cedric's aspeed-4.2 qemu tree - do you mind testing it out? If those patches
> resolve the issue Maybe we could point Guenter at that tree, though really we
> should get the fixes upstream so this isn't an issue.
>
> [1] https://github.com/legoater/qemu/compare/59dda66ab756e52e6a9c1ef89660d30b3769f63c...aspeed-4.2
>
Okay. I'll give it a try.
Thanks,
Jae
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-04 23:50 ` Brendan Higgins
@ 2019-09-05 0:56 ` Jae Hyun Yoo
0 siblings, 0 replies; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-05 0:56 UTC (permalink / raw)
To: Brendan Higgins, Joel Stanley
Cc: Ryan Chen, Andrew Jeffery, OpenBMC Maillist, Tao Ren
On 9/4/2019 4:50 PM, Brendan Higgins wrote:
> On Wed, Sep 4, 2019 at 3:55 PM Joel Stanley <joel@jms.id.au> wrote:
>>
>> Hi Jae,
>>
>> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> 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.
>>>
>>> By applying this change, SDA data-low and SCL clock-low timeout feature
>>> also could be enabled which was disabled previously.
>>
>> Please consider sending your RFC patches to the upstream list. You
>> have a big backlog of patches now.
>
> Joel, thanks for keeping track of this!
>
> Sorry, for not keeping up with my emails, I don't think I have time to
> continue to maintain this.
>
> However, I don't want to hand this off in a bad state. I will try to
> get up to date on all the emails in the coming weeks.
>
> Jae, can you start sending things upstream as Joel suggested?
Sure, I'll start upstreaming after taking some tests on Cedric's QEMU
patch.
Thanks,
Jae
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-04 20:07 ` [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive " Jae Hyun Yoo
2019-09-04 22:37 ` Tao Ren
@ 2019-09-05 22:28 ` Tao Ren
2019-09-05 22:48 ` Jae Hyun Yoo
1 sibling, 1 reply; 23+ messages in thread
From: Tao Ren @ 2019-09-05 22:28 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, Ryan Chen
Cc: openbmc
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@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);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-05 22:28 ` Tao Ren
@ 2019-09-05 22:48 ` Jae Hyun Yoo
2019-09-05 23:19 ` Tao Ren
0 siblings, 1 reply; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-05 22:48 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Ryan Chen
Cc: openbmc
Hi Tao,
On 9/5/2019 3:28 PM, Tao Ren wrote:
> 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@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.
Basic concept of this implementation is setting the slave state of
driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
aspeed_i2c_reset() directly from interrupt context. Instead, when a
master xfer happens after that, it will try bus recovery
through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
if needed.
If this patch doesn't work in your case, test it again after adding
one line code into this driver. See below.
>
>> ---
>> 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;
aspeed_i2c_reset(bus);
I didn't add it in this patch because I wanted to avoid calling of this
reset function from interrupt context but give it a try.
Thanks,
Jae
>> + 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);
>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-05 22:48 ` Jae Hyun Yoo
@ 2019-09-05 23:19 ` Tao Ren
2019-09-05 23:35 ` Jae Hyun Yoo
0 siblings, 1 reply; 23+ messages in thread
From: Tao Ren @ 2019-09-05 23:19 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, Ryan Chen
Cc: openbmc
On 9/5/19 3:48 PM, Jae Hyun Yoo wrote:
> Hi Tao,
>
> On 9/5/2019 3:28 PM, Tao Ren wrote:
>> 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@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.
>
> Basic concept of this implementation is setting the slave state of
> driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
> aspeed_i2c_reset() directly from interrupt context. Instead, when a
> master xfer happens after that, it will try bus recovery
> through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
> if needed.
>
> If this patch doesn't work in your case, test it again after adding
> one line code into this driver. See below.
If bus_reset is really needed in this case, then I'd prefer to do it immediately or in a threaded_irq_handler if it's bad idea to call aspeed_i2c_reset() in hardware interrupt context. The reasons being:
1) we don't know when userspace starts next master transfer.
2) aspeed_i2c_recover_bus() is not triggered in aspeed_i2c_master_xfer() in my environment because bus is "idle" (I2CD14[16] == 0).
Thanks,
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;
>
> aspeed_i2c_reset(bus);
>
> I didn't add it in this patch because I wanted to avoid calling of this
> reset function from interrupt context but give it a try.
>
> Thanks,
> Jae
I believe this will solve my problem, but let me test it and will share you results later.
>>> + 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);
>>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-05 23:19 ` Tao Ren
@ 2019-09-05 23:35 ` Jae Hyun Yoo
2019-09-06 1:16 ` Tao Ren
0 siblings, 1 reply; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-05 23:35 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Ryan Chen
Cc: openbmc
On 9/5/2019 4:19 PM, Tao Ren wrote:
> On 9/5/19 3:48 PM, Jae Hyun Yoo wrote:
>> Hi Tao,
>>
>> On 9/5/2019 3:28 PM, Tao Ren wrote:
>>> 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@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.
>>
>> Basic concept of this implementation is setting the slave state of
>> driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
>> aspeed_i2c_reset() directly from interrupt context. Instead, when a
>> master xfer happens after that, it will try bus recovery
>> through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
>> if needed.
>>
>> If this patch doesn't work in your case, test it again after adding
>> one line code into this driver. See below.
>
> If bus_reset is really needed in this case, then I'd prefer to do it immediately or in a threaded_irq_handler if it's bad idea to call aspeed_i2c_reset() in hardware interrupt context. The reasons being:
>
> 1) we don't know when userspace starts next master transfer.
> 2) aspeed_i2c_recover_bus() is not triggered in aspeed_i2c_master_xfer() in my environment because bus is "idle" (I2CD14[16] == 0).
>
Oh, so to support the case as well, probably we need to add a flag for
indicating recovery needs when a master xfer comes then it could
forcibly recover and reset the bus even if the bus is idle. Can you
please test that with making code changes? Unfortunately, I can't
reproduce the case in my system.
>>
>>>
>>>> ---
>>>> 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;
>>
>> aspeed_i2c_reset(bus);
>>
>> I didn't add it in this patch because I wanted to avoid calling of this
>> reset function from interrupt context but give it a try.
>>
>> Thanks,
>> Jae
>
> I believe this will solve my problem, but let me test it and will share you results later.
Please share this result too. It would be useful to complete making this
patch.
Thanks,
Jae
>>>> + 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);
>>>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-05 23:35 ` Jae Hyun Yoo
@ 2019-09-06 1:16 ` Tao Ren
2019-09-06 1:56 ` [Potential Spoof] " Tao Ren
0 siblings, 1 reply; 23+ messages in thread
From: Tao Ren @ 2019-09-06 1:16 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, Ryan Chen
Cc: openbmc
Hi Jae,
On 9/5/19 4:35 PM, Jae Hyun Yoo wrote:
> On 9/5/2019 4:19 PM, Tao Ren wrote:
>> On 9/5/19 3:48 PM, Jae Hyun Yoo wrote:
>>> Hi Tao,
>>>
>>> On 9/5/2019 3:28 PM, Tao Ren wrote:
>>>> 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@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.
>>>
>>> Basic concept of this implementation is setting the slave state of
>>> driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
>>> aspeed_i2c_reset() directly from interrupt context. Instead, when a
>>> master xfer happens after that, it will try bus recovery
>>> through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
>>> if needed.
>>>
>>> If this patch doesn't work in your case, test it again after adding
>>> one line code into this driver. See below.
>>
>> If bus_reset is really needed in this case, then I'd prefer to do it immediately or in a threaded_irq_handler if it's bad idea to call aspeed_i2c_reset() in hardware interrupt context. The reasons being:
>>
>> 1) we don't know when userspace starts next master transfer.
>> 2) aspeed_i2c_recover_bus() is not triggered in aspeed_i2c_master_xfer() in my environment because bus is "idle" (I2CD14[16] == 0).
>>
>
> Oh, so to support the case as well, probably we need to add a flag for
> indicating recovery needs when a master xfer comes then it could
> forcibly recover and reset the bus even if the bus is idle. Can you
> please test that with making code changes? Unfortunately, I can't
> reproduce the case in my system.
Not sure if I understand it correctly, but given we already reset the bus in interrupt handler, the extra flag should not be needed?
>>>
>>>>
>>>>> ---
>>>>> 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;
>>>
>>> aspeed_i2c_reset(bus);
>>>
>>> I didn't add it in this patch because I wanted to avoid calling of this
>>> reset function from interrupt context but give it a try.
>>>
>>> Thanks,
>>> Jae
>>
>> I believe this will solve my problem, but let me test it and will share you results later.
>
> Please share this result too. It would be useful to complete making this
> patch.
>
> Thanks,
> Jae
Adding aspeed_i2c_reset(bus) does fix my problem. Thank you Jae.
Cheers,
Tao
>>>>> + 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);
>>>>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Potential Spoof] Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-06 1:16 ` Tao Ren
@ 2019-09-06 1:56 ` Tao Ren
2019-09-06 2:35 ` Tao Ren
0 siblings, 1 reply; 23+ messages in thread
From: Tao Ren @ 2019-09-06 1:56 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, Ryan Chen
Cc: openbmc
On 9/5/19 6:16 PM, Tao Ren wrote:
> Hi Jae,
>
> On 9/5/19 4:35 PM, Jae Hyun Yoo wrote:
>> On 9/5/2019 4:19 PM, Tao Ren wrote:
>>> On 9/5/19 3:48 PM, Jae Hyun Yoo wrote:
>>>> Hi Tao,
>>>>
>>>> On 9/5/2019 3:28 PM, Tao Ren wrote:
>>>>> 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@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.
>>>>
>>>> Basic concept of this implementation is setting the slave state of
>>>> driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
>>>> aspeed_i2c_reset() directly from interrupt context. Instead, when a
>>>> master xfer happens after that, it will try bus recovery
>>>> through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
>>>> if needed.
>>>>
>>>> If this patch doesn't work in your case, test it again after adding
>>>> one line code into this driver. See below.
>>>
>>> If bus_reset is really needed in this case, then I'd prefer to do it immediately or in a threaded_irq_handler if it's bad idea to call aspeed_i2c_reset() in hardware interrupt context. The reasons being:
>>>
>>> 1) we don't know when userspace starts next master transfer.
>>> 2) aspeed_i2c_recover_bus() is not triggered in aspeed_i2c_master_xfer() in my environment because bus is "idle" (I2CD14[16] == 0).
>>>
>>
>> Oh, so to support the case as well, probably we need to add a flag for
>> indicating recovery needs when a master xfer comes then it could
>> forcibly recover and reset the bus even if the bus is idle. Can you
>> please test that with making code changes? Unfortunately, I can't
>> reproduce the case in my system.
>
> Not sure if I understand it correctly, but given we already reset the bus in interrupt handler, the extra flag should not be needed?
One of my colleagues reminded me to enable bus auto release (I2CD00[17]) which could avoid explicit bus reset in interrupt handler.
Let me try and will update back soon.
Thanks,
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;
>>>>
>>>> aspeed_i2c_reset(bus);
>>>>
>>>> I didn't add it in this patch because I wanted to avoid calling of this
>>>> reset function from interrupt context but give it a try.
>>>>
>>>> Thanks,
>>>> Jae
>>>
>>> I believe this will solve my problem, but let me test it and will share you results later.
>>
>> Please share this result too. It would be useful to complete making this
>> patch.
>>
>> Thanks,
>> Jae
>
> Adding aspeed_i2c_reset(bus) does fix my problem. Thank you Jae.
>
>
> Cheers,
>
> Tao
>
>>>>>> + 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);
>>>>>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-06 1:56 ` [Potential Spoof] " Tao Ren
@ 2019-09-06 2:35 ` Tao Ren
2019-09-06 16:20 ` Jae Hyun Yoo
0 siblings, 1 reply; 23+ messages in thread
From: Tao Ren @ 2019-09-06 2:35 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, Ryan Chen
Cc: openbmc
Hi Jae,
On 9/5/19 6:56 PM, Tao Ren wrote:
> On 9/5/19 6:16 PM, Tao Ren wrote:
>> Hi Jae,
>>
>> On 9/5/19 4:35 PM, Jae Hyun Yoo wrote:
>>> On 9/5/2019 4:19 PM, Tao Ren wrote:
>>>> On 9/5/19 3:48 PM, Jae Hyun Yoo wrote:
>>>>> Hi Tao,
>>>>>
>>>>> On 9/5/2019 3:28 PM, Tao Ren wrote:
>>>>>> 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@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.
>>>>>
>>>>> Basic concept of this implementation is setting the slave state of
>>>>> driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
>>>>> aspeed_i2c_reset() directly from interrupt context. Instead, when a
>>>>> master xfer happens after that, it will try bus recovery
>>>>> through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
>>>>> if needed.
>>>>>
>>>>> If this patch doesn't work in your case, test it again after adding
>>>>> one line code into this driver. See below.
>>>>
>>>> If bus_reset is really needed in this case, then I'd prefer to do it immediately or in a threaded_irq_handler if it's bad idea to call aspeed_i2c_reset() in hardware interrupt context. The reasons being:
>>>>
>>>> 1) we don't know when userspace starts next master transfer.
>>>> 2) aspeed_i2c_recover_bus() is not triggered in aspeed_i2c_master_xfer() in my environment because bus is "idle" (I2CD14[16] == 0).
>>>>
>>>
>>> Oh, so to support the case as well, probably we need to add a flag for
>>> indicating recovery needs when a master xfer comes then it could
>>> forcibly recover and reset the bus even if the bus is idle. Can you
>>> please test that with making code changes? Unfortunately, I can't
>>> reproduce the case in my system.
>>
>> Not sure if I understand it correctly, but given we already reset the bus in interrupt handler, the extra flag should not be needed?
>
> One of my colleagues reminded me to enable bus auto release (I2CD00[17]) which could avoid explicit bus reset in interrupt handler.
>
> Let me try and will update back soon.
Let me summarize my testing result on Minipack BMC:
BMC i2c controller cannot exit slave state by enabling slave_inactive_timeout interrupt only: in this case, i2c controller stays in SRXD state and slave_inactive_timeout interrupt will be generated again and again until bus is explicitly reset.
I've tried below 2 approaches and both works on my platform:
- calling aspeed_i2c_reset() in interrupt handler when slave_inacive_timeout interrupt is delivered.
- enabling bus auto release feature (I2CD00[17]) in probe function.
Cheers,
Tao
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support
2019-09-06 2:35 ` Tao Ren
@ 2019-09-06 16:20 ` Jae Hyun Yoo
0 siblings, 0 replies; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-06 16:20 UTC (permalink / raw)
To: Tao Ren, Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, Ryan Chen
Cc: openbmc
Hi Tao,
On 9/5/2019 7:35 PM, Tao Ren wrote:
> Hi Jae,
>
> On 9/5/19 6:56 PM, Tao Ren wrote:
>> On 9/5/19 6:16 PM, Tao Ren wrote:
>>> Hi Jae,
>>>
>>> On 9/5/19 4:35 PM, Jae Hyun Yoo wrote:
>>>> On 9/5/2019 4:19 PM, Tao Ren wrote:
>>>>> On 9/5/19 3:48 PM, Jae Hyun Yoo wrote:
>>>>>> Hi Tao,
>>>>>>
>>>>>> On 9/5/2019 3:28 PM, Tao Ren wrote:
>>>>>>> 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@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.
>>>>>>
>>>>>> Basic concept of this implementation is setting the slave state of
>>>>>> driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
>>>>>> aspeed_i2c_reset() directly from interrupt context. Instead, when a
>>>>>> master xfer happens after that, it will try bus recovery
>>>>>> through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
>>>>>> if needed.
>>>>>>
>>>>>> If this patch doesn't work in your case, test it again after adding
>>>>>> one line code into this driver. See below.
>>>>>
>>>>> If bus_reset is really needed in this case, then I'd prefer to do it immediately or in a threaded_irq_handler if it's bad idea to call aspeed_i2c_reset() in hardware interrupt context. The reasons being:
>>>>>
>>>>> 1) we don't know when userspace starts next master transfer.
>>>>> 2) aspeed_i2c_recover_bus() is not triggered in aspeed_i2c_master_xfer() in my environment because bus is "idle" (I2CD14[16] == 0).
>>>>>
>>>>
>>>> Oh, so to support the case as well, probably we need to add a flag for
>>>> indicating recovery needs when a master xfer comes then it could
>>>> forcibly recover and reset the bus even if the bus is idle. Can you
>>>> please test that with making code changes? Unfortunately, I can't
>>>> reproduce the case in my system.
>>>
>>> Not sure if I understand it correctly, but given we already reset the bus in interrupt handler, the extra flag should not be needed?
>>
>> One of my colleagues reminded me to enable bus auto release (I2CD00[17]) which could avoid explicit bus reset in interrupt handler.
>>
>> Let me try and will update back soon.
>
> Let me summarize my testing result on Minipack BMC:
>
> BMC i2c controller cannot exit slave state by enabling slave_inactive_timeout interrupt only: in this case, i2c controller stays in SRXD state and slave_inactive_timeout interrupt will be generated again and again until bus is explicitly reset.
>
> I've tried below 2 approaches and both works on my platform:
> - calling aspeed_i2c_reset() in interrupt handler when slave_inacive_timeout interrupt is delivered.
> - enabling bus auto release feature (I2CD00[17]) in probe function.
Thanks a lot for your testing result and suggestions. It seems that the
latter way - enabling bus auto release would be better for this
implementation.
I'll submit a next spin today.
Cheers,
Jae
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-05 0:54 ` Jae Hyun Yoo
@ 2019-09-11 18:56 ` Jae Hyun Yoo
2019-09-12 1:22 ` Andrew Jeffery
0 siblings, 1 reply; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-11 18:56 UTC (permalink / raw)
To: Andrew Jeffery, Joel Stanley, Cedric Le Goater, Eddie James
Cc: Ryan Chen, OpenBMC Maillist, Brendan Higgins, Tao Ren
Hi Andrew,
On 9/4/2019 5:54 PM, Jae Hyun Yoo wrote:
> On 9/4/2019 5:10 PM, Andrew Jeffery wrote:
>>
>>
>> On Thu, 5 Sep 2019, at 09:10, Jae Hyun Yoo wrote:
>>> Hi Andrew,
>>>
>>> On 9/4/2019 4:12 PM, Andrew Jeffery wrote:
>>>> On Thu, 5 Sep 2019, at 08:31, Jae Hyun Yoo wrote:
>>>>> Hi Joel,
>>>>>
>>>>> On 9/4/2019 3:54 PM, Joel Stanley wrote:
>>>>>> Hi Jae,
>>>>>>
>>>>>> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo
>>>>>> <jae.hyun.yoo@linux.intel.com> 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.
>>>>>>>
>>>>>>> By applying this change, SDA data-low and SCL clock-low timeout
>>>>>>> feature
>>>>>>> also could be enabled which was disabled previously.
>>>>>>
>>>>>> Please consider sending your RFC patches to the upstream list. You
>>>>>> have a big backlog of patches now.
>>>>>
>>>>> Thanks for the reminding. I can't send the RFC patches yet because
>>>>> QEMU
>>>>> H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
>>>>
>>>> QEMU shouldn't be preventing you from sending patches upstream, rather
>>>> it prevents us from enabling the buffer mode support in the OpenBMC
>>>> kernel tree. You should be sending all patches upstream as early as
>>>> possible.
>>>
>>> I met a QEMU issue when I was upstreaming a patch set last year:
>>> https://lists.ozlabs.org/pipermail/linux-aspeed/2018-September/000750.html
>>>
>>>
>>> If OpenBMC community accepts the QEMU issue, I can submit the RFC
>>> patches to upstream. Will submit the patch set soon to linux tree.
>>
>> Ah, didn't realise it was Guenter that ran into it. We have some
>> changes[1] in
>> Cedric's aspeed-4.2 qemu tree - do you mind testing it out? If those
>> patches
>> resolve the issue Maybe we could point Guenter at that tree, though
>> really we
>> should get the fixes upstream so this isn't an issue.
>>
>> [1]
>> https://github.com/legoater/qemu/compare/59dda66ab756e52e6a9c1ef89660d30b3769f63c...aspeed-4.2
>>
>>
>
> Okay. I'll give it a try.
I've tested I2C buffer mode support in QEMU using:
git://github.com/legoater/qemu.git
SRCBRANCH = "aspeed-4.2"
SRCREV = "1b31d645c448858eb7d11d463a4cb77df0ee7923"
Checked that I2C buffer mode works on the latest QEMU H/W model.
Thanks Cedric, Eddie for the H/W model implementation.
I'll submit all I2C backlog patches into linux upstream.
Cheers,
Jae
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-11 18:56 ` Jae Hyun Yoo
@ 2019-09-12 1:22 ` Andrew Jeffery
2019-09-12 1:26 ` Jae Hyun Yoo
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Jeffery @ 2019-09-12 1:22 UTC (permalink / raw)
To: Jae Hyun Yoo, Joel Stanley, Cédric Le Goater, Eddie James
Cc: Ryan Chen, OpenBMC Maillist, Brendan Higgins, Tao Ren
On Thu, 12 Sep 2019, at 04:26, Jae Hyun Yoo wrote:
> Hi Andrew,
>
> On 9/4/2019 5:54 PM, Jae Hyun Yoo wrote:
> > On 9/4/2019 5:10 PM, Andrew Jeffery wrote:
> >>
> >>
> >> On Thu, 5 Sep 2019, at 09:10, Jae Hyun Yoo wrote:
> >>> Hi Andrew,
> >>>
> >>> On 9/4/2019 4:12 PM, Andrew Jeffery wrote:
> >>>> On Thu, 5 Sep 2019, at 08:31, Jae Hyun Yoo wrote:
> >>>>> Hi Joel,
> >>>>>
> >>>>> On 9/4/2019 3:54 PM, Joel Stanley wrote:
> >>>>>> Hi Jae,
> >>>>>>
> >>>>>> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo
> >>>>>> <jae.hyun.yoo@linux.intel.com> 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.
> >>>>>>>
> >>>>>>> By applying this change, SDA data-low and SCL clock-low timeout
> >>>>>>> feature
> >>>>>>> also could be enabled which was disabled previously.
> >>>>>>
> >>>>>> Please consider sending your RFC patches to the upstream list. You
> >>>>>> have a big backlog of patches now.
> >>>>>
> >>>>> Thanks for the reminding. I can't send the RFC patches yet because
> >>>>> QEMU
> >>>>> H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
> >>>>
> >>>> QEMU shouldn't be preventing you from sending patches upstream, rather
> >>>> it prevents us from enabling the buffer mode support in the OpenBMC
> >>>> kernel tree. You should be sending all patches upstream as early as
> >>>> possible.
> >>>
> >>> I met a QEMU issue when I was upstreaming a patch set last year:
> >>> https://lists.ozlabs.org/pipermail/linux-aspeed/2018-September/000750.html
> >>>
> >>>
> >>> If OpenBMC community accepts the QEMU issue, I can submit the RFC
> >>> patches to upstream. Will submit the patch set soon to linux tree.
> >>
> >> Ah, didn't realise it was Guenter that ran into it. We have some
> >> changes[1] in
> >> Cedric's aspeed-4.2 qemu tree - do you mind testing it out? If those
> >> patches
> >> resolve the issue Maybe we could point Guenter at that tree, though
> >> really we
> >> should get the fixes upstream so this isn't an issue.
> >>
> >> [1]
> >> https://github.com/legoater/qemu/compare/59dda66ab756e52e6a9c1ef89660d30b3769f63c...aspeed-4.2
> >>
> >>
> >
> > Okay. I'll give it a try.
>
> I've tested I2C buffer mode support in QEMU using:
> git://github.com/legoater/qemu.git
> SRCBRANCH = "aspeed-4.2"
> SRCREV = "1b31d645c448858eb7d11d463a4cb77df0ee7923"
This looks like changes to the qemu bitbake recipe? Have you
integrated openbmc/qemu into Yocto's runqemu infrastructure,
or were you just using bitbake to build qemu? I'd love to see
patches if you've done the former :)
Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support
2019-09-12 1:22 ` Andrew Jeffery
@ 2019-09-12 1:26 ` Jae Hyun Yoo
0 siblings, 0 replies; 23+ messages in thread
From: Jae Hyun Yoo @ 2019-09-12 1:26 UTC (permalink / raw)
To: Andrew Jeffery, Joel Stanley, Cédric Le Goater, Eddie James
Cc: Brendan Higgins, OpenBMC Maillist, Ryan Chen, Tao Ren
On 9/11/2019 6:22 PM, Andrew Jeffery wrote:
>
>
> On Thu, 12 Sep 2019, at 04:26, Jae Hyun Yoo wrote:
>> Hi Andrew,
>>
>> On 9/4/2019 5:54 PM, Jae Hyun Yoo wrote:
>>> On 9/4/2019 5:10 PM, Andrew Jeffery wrote:
>>>>
>>>>
>>>> On Thu, 5 Sep 2019, at 09:10, Jae Hyun Yoo wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> On 9/4/2019 4:12 PM, Andrew Jeffery wrote:
>>>>>> On Thu, 5 Sep 2019, at 08:31, Jae Hyun Yoo wrote:
>>>>>>> Hi Joel,
>>>>>>>
>>>>>>> On 9/4/2019 3:54 PM, Joel Stanley wrote:
>>>>>>>> Hi Jae,
>>>>>>>>
>>>>>>>> On Wed, 4 Sep 2019 at 20:08, Jae Hyun Yoo
>>>>>>>> <jae.hyun.yoo@linux.intel.com> 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.
>>>>>>>>>
>>>>>>>>> By applying this change, SDA data-low and SCL clock-low timeout
>>>>>>>>> feature
>>>>>>>>> also could be enabled which was disabled previously.
>>>>>>>>
>>>>>>>> Please consider sending your RFC patches to the upstream list. You
>>>>>>>> have a big backlog of patches now.
>>>>>>>
>>>>>>> Thanks for the reminding. I can't send the RFC patches yet because
>>>>>>> QEMU
>>>>>>> H/W model isn't ready yet. I'm still waiting for the fix from Cedric.
>>>>>>
>>>>>> QEMU shouldn't be preventing you from sending patches upstream, rather
>>>>>> it prevents us from enabling the buffer mode support in the OpenBMC
>>>>>> kernel tree. You should be sending all patches upstream as early as
>>>>>> possible.
>>>>>
>>>>> I met a QEMU issue when I was upstreaming a patch set last year:
>>>>> https://lists.ozlabs.org/pipermail/linux-aspeed/2018-September/000750.html
>>>>>
>>>>>
>>>>> If OpenBMC community accepts the QEMU issue, I can submit the RFC
>>>>> patches to upstream. Will submit the patch set soon to linux tree.
>>>>
>>>> Ah, didn't realise it was Guenter that ran into it. We have some
>>>> changes[1] in
>>>> Cedric's aspeed-4.2 qemu tree - do you mind testing it out? If those
>>>> patches
>>>> resolve the issue Maybe we could point Guenter at that tree, though
>>>> really we
>>>> should get the fixes upstream so this isn't an issue.
>>>>
>>>> [1]
>>>> https://github.com/legoater/qemu/compare/59dda66ab756e52e6a9c1ef89660d30b3769f63c...aspeed-4.2
>>>>
>>>>
>>>
>>> Okay. I'll give it a try.
>>
>> I've tested I2C buffer mode support in QEMU using:
>> git://github.com/legoater/qemu.git
>> SRCBRANCH = "aspeed-4.2"
>> SRCREV = "1b31d645c448858eb7d11d463a4cb77df0ee7923"
>
> This looks like changes to the qemu bitbake recipe? Have you
> integrated openbmc/qemu into Yocto's runqemu infrastructure,
> or were you just using bitbake to build qemu? I'd love to see
> patches if you've done the former :)
>
> Andrew
>
No, I just made this change using a bbappend in my local yocto tree.
The source revision on the branch worked well.
Jae
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-09-12 1:26 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04 20:07 [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W timeout support Jae Hyun Yoo
2019-09-04 20:07 ` [PATCH dev-5.2 1/2] dt-bindings: i2c: aspeed: add hardware " Jae Hyun Yoo
2019-09-04 20:07 ` [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive " Jae Hyun Yoo
2019-09-04 22:37 ` Tao Ren
2019-09-05 22:28 ` Tao Ren
2019-09-05 22:48 ` Jae Hyun Yoo
2019-09-05 23:19 ` Tao Ren
2019-09-05 23:35 ` Jae Hyun Yoo
2019-09-06 1:16 ` Tao Ren
2019-09-06 1:56 ` [Potential Spoof] " Tao Ren
2019-09-06 2:35 ` Tao Ren
2019-09-06 16:20 ` Jae Hyun Yoo
2019-09-04 22:54 ` [PATCH dev-5.2 0/2] i2c: aspeed: Add H/W " Joel Stanley
2019-09-04 23:01 ` Jae Hyun Yoo
2019-09-04 23:12 ` Andrew Jeffery
2019-09-04 23:40 ` Jae Hyun Yoo
2019-09-05 0:10 ` Andrew Jeffery
2019-09-05 0:54 ` Jae Hyun Yoo
2019-09-11 18:56 ` Jae Hyun Yoo
2019-09-12 1:22 ` Andrew Jeffery
2019-09-12 1:26 ` Jae Hyun Yoo
2019-09-04 23:50 ` Brendan Higgins
2019-09-05 0:56 ` Jae Hyun Yoo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.