* [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller @ 2015-11-05 12:34 Xiangliang Yu 2015-11-05 13:52 ` Mika Westerberg 0 siblings, 1 reply; 8+ messages in thread From: Xiangliang Yu @ 2015-11-05 12:34 UTC (permalink / raw) To: andriy.shevchenko, jarkko.nikula, mika.westerberg, wsa, linux-i2c, linux-kernel Cc: Ken.Xue, Vincent.Wan, Ray.Huang, Annie.Wang, Tony.Li, Xiangliang Yu Because of some hardware limitation, AMD I2C controller can't trigger next interrupt if interrupt status has been changed after clearing interrupt status bits. Then, I2C will lost interrupt and IO timeout. According to hardware design, this patch implements a workaround to disable i2c controller interrupt after clearing interrupt bits when entering ISR and to enable i2c interrupt before exiting ISR. To reduce the performance impacts on other vendors, use unlikely function to check flag in ISR. Signed-off-by: Xiangliang Yu <Xiangliang.Yu@amd.com> --- drivers/i2c/busses/i2c-designware-core.c | 6 ++++++ drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++++ 3 files changed, 11 insertions(+) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 7441cdc..04e7b65 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) stat = i2c_dw_read_clear_intrbits(dev); + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) + i2c_dw_disable_int(dev); + if (stat & DW_IC_INTR_TX_ABRT) { dev->cmd_err |= DW_IC_ERR_TX_ABRT; dev->status = STATUS_IDLE; @@ -811,6 +814,9 @@ tx_aborted: if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err) complete(&dev->cmd_complete); + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) + dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK); + return IRQ_HANDLED; } EXPORT_SYMBOL_GPL(i2c_dw_isr); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 9630222..808ef6a 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -111,6 +111,7 @@ struct dw_i2c_dev { #define ACCESS_SWAP 0x00000001 #define ACCESS_16BIT 0x00000002 +#define ACCESS_INTR_MASK 0x00000004 extern u32 dw_readl(struct dw_i2c_dev *dev, int offset); extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 472b882..0c59357 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -251,6 +251,10 @@ static int dw_i2c_probe(struct platform_device *pdev) dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1; dev->adapter.nr = pdev->id; } + + if (!strncmp(pdev->name, "AMD0010", 7)) + dev->accessor_flags |= ACCESS_INTR_MASK; + r = i2c_dw_init(dev); if (r) return r; -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller 2015-11-05 12:34 [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller Xiangliang Yu @ 2015-11-05 13:52 ` Mika Westerberg 2015-11-06 4:34 ` Yu, Xiangliang 0 siblings, 1 reply; 8+ messages in thread From: Mika Westerberg @ 2015-11-05 13:52 UTC (permalink / raw) To: Xiangliang Yu Cc: andriy.shevchenko, jarkko.nikula, wsa, linux-i2c, linux-kernel, Ken.Xue, Vincent.Wan, Ray.Huang, Annie.Wang, Tony.Li On Thu, Nov 05, 2015 at 08:34:44PM +0800, Xiangliang Yu wrote: > Because of some hardware limitation, AMD I2C controller can't > trigger next interrupt if interrupt status has been changed > after clearing interrupt status bits. Then, I2C will lost > interrupt and IO timeout. > > According to hardware design, this patch implements a workaround > to disable i2c controller interrupt after clearing interrupt bits > when entering ISR and to enable i2c interrupt before exiting ISR. > > To reduce the performance impacts on other vendors, use unlikely > function to check flag in ISR. > > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@amd.com> > --- > drivers/i2c/busses/i2c-designware-core.c | 6 ++++++ > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++++ > 3 files changed, 11 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c > index 7441cdc..04e7b65 100644 > --- a/drivers/i2c/busses/i2c-designware-core.c > +++ b/drivers/i2c/busses/i2c-designware-core.c > @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) > > stat = i2c_dw_read_clear_intrbits(dev); What if the status changes right here, before you go and mask the interrupt? > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > + i2c_dw_disable_int(dev); > + > if (stat & DW_IC_INTR_TX_ABRT) { > dev->cmd_err |= DW_IC_ERR_TX_ABRT; > dev->status = STATUS_IDLE; > @@ -811,6 +814,9 @@ tx_aborted: > if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err) > complete(&dev->cmd_complete); > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > + dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK); The driver disables TX interrupt when it is not needed anymore or when TX gets aborted but the above will re-enable all interrupts regardless. Is that the intention? > + > return IRQ_HANDLED; > } > EXPORT_SYMBOL_GPL(i2c_dw_isr); > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h > index 9630222..808ef6a 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -111,6 +111,7 @@ struct dw_i2c_dev { > > #define ACCESS_SWAP 0x00000001 > #define ACCESS_16BIT 0x00000002 > +#define ACCESS_INTR_MASK 0x00000004 > > extern u32 dw_readl(struct dw_i2c_dev *dev, int offset); > extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 472b882..0c59357 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -251,6 +251,10 @@ static int dw_i2c_probe(struct platform_device *pdev) > dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1; > dev->adapter.nr = pdev->id; > } > + > + if (!strncmp(pdev->name, "AMD0010", 7)) > + dev->accessor_flags |= ACCESS_INTR_MASK; > + Can't you put this to ->driver_data? For example it could refer to a configuration structure that then contains quirk flags. > r = i2c_dw_init(dev); > if (r) > return r; > -- > 1.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller 2015-11-05 13:52 ` Mika Westerberg @ 2015-11-06 4:34 ` Yu, Xiangliang 2015-11-06 7:46 ` Mika Westerberg 2015-11-06 8:59 ` Jarkko Nikula 0 siblings, 2 replies; 8+ messages in thread From: Yu, Xiangliang @ 2015-11-06 4:34 UTC (permalink / raw) To: Mika Westerberg Cc: andriy.shevchenko@linux.intel.com, jarkko.nikula@linux.intel.com, wsa@the-dreams.de, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Xue, Ken, Wan, Vincent, Huang, Ray, Wang, Annie, Li, Tony > -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Thursday, November 05, 2015 9:52 PM > To: Yu, Xiangliang > Cc: andriy.shevchenko@linux.intel.com; jarkko.nikula@linux.intel.com; > wsa@the-dreams.de; linux-i2c@vger.kernel.org; linux- > kernel@vger.kernel.org; Xue, Ken; Wan, Vincent; Huang, Ray; Wang, Annie; > Li, Tony > Subject: Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD > controller > > On Thu, Nov 05, 2015 at 08:34:44PM +0800, Xiangliang Yu wrote: > > Because of some hardware limitation, AMD I2C controller can't trigger > > next interrupt if interrupt status has been changed after clearing > > interrupt status bits. Then, I2C will lost interrupt and IO timeout. > > > > According to hardware design, this patch implements a workaround to > > disable i2c controller interrupt after clearing interrupt bits when > > entering ISR and to enable i2c interrupt before exiting ISR. > > > > To reduce the performance impacts on other vendors, use unlikely > > function to check flag in ISR. > > > > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@amd.com> > > --- > > drivers/i2c/busses/i2c-designware-core.c | 6 ++++++ > > drivers/i2c/busses/i2c-designware-core.h | 1 + > > drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++++ > > 3 files changed, 11 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-designware-core.c > > b/drivers/i2c/busses/i2c-designware-core.c > > index 7441cdc..04e7b65 100644 > > --- a/drivers/i2c/busses/i2c-designware-core.c > > +++ b/drivers/i2c/busses/i2c-designware-core.c > > @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) > > > > stat = i2c_dw_read_clear_intrbits(dev); > > What if the status changes right here, before you go and mask the interrupt? Have no effect, because i2c controller can't trigger next interrupt. > > > > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > > + i2c_dw_disable_int(dev); > > + > > if (stat & DW_IC_INTR_TX_ABRT) { > > dev->cmd_err |= DW_IC_ERR_TX_ABRT; > > dev->status = STATUS_IDLE; > > @@ -811,6 +814,9 @@ tx_aborted: > > if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || > dev->msg_err) > > complete(&dev->cmd_complete); > > > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > > + dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, > DW_IC_INTR_MASK); > > The driver disables TX interrupt when it is not needed anymore or when TX > gets aborted but the above will re-enable all interrupts regardless. > Is that the intention? No, i2c controller can trigger next interrupt only after re-enable all interrupt. > > > + > > return IRQ_HANDLED; > > } > > EXPORT_SYMBOL_GPL(i2c_dw_isr); > > diff --git a/drivers/i2c/busses/i2c-designware-core.h > > b/drivers/i2c/busses/i2c-designware-core.h > > index 9630222..808ef6a 100644 > > --- a/drivers/i2c/busses/i2c-designware-core.h > > +++ b/drivers/i2c/busses/i2c-designware-core.h > > @@ -111,6 +111,7 @@ struct dw_i2c_dev { > > > > #define ACCESS_SWAP 0x00000001 > > #define ACCESS_16BIT 0x00000002 > > +#define ACCESS_INTR_MASK 0x00000004 > > > > extern u32 dw_readl(struct dw_i2c_dev *dev, int offset); extern void > > dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); diff --git > > a/drivers/i2c/busses/i2c-designware-platdrv.c > > b/drivers/i2c/busses/i2c-designware-platdrv.c > > index 472b882..0c59357 100644 > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > > @@ -251,6 +251,10 @@ static int dw_i2c_probe(struct platform_device > *pdev) > > dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1; > > dev->adapter.nr = pdev->id; > > } > > + > > + if (!strncmp(pdev->name, "AMD0010", 7)) > > + dev->accessor_flags |= ACCESS_INTR_MASK; > > + > > Can't you put this to ->driver_data? For example it could refer to a > configuration structure that then contains quirk flags. I think it will need to add a function to support it, but the function Is rarely used. It will easy to add if i2c driver has quirk mechanisms. Added code is very simple and have no influence on others. > > > r = i2c_dw_init(dev); > > if (r) > > return r; > > -- > > 1.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller 2015-11-06 4:34 ` Yu, Xiangliang @ 2015-11-06 7:46 ` Mika Westerberg 2015-11-06 8:33 ` Yu, Xiangliang 2015-11-06 8:59 ` Jarkko Nikula 1 sibling, 1 reply; 8+ messages in thread From: Mika Westerberg @ 2015-11-06 7:46 UTC (permalink / raw) To: Yu, Xiangliang Cc: andriy.shevchenko@linux.intel.com, jarkko.nikula@linux.intel.com, wsa@the-dreams.de, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Xue, Ken, Wan, Vincent, Huang, Ray, Wang, Annie, Li, Tony On Fri, Nov 06, 2015 at 04:34:19AM +0000, Yu, Xiangliang wrote: > > -----Original Message----- > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > Sent: Thursday, November 05, 2015 9:52 PM > > To: Yu, Xiangliang > > Cc: andriy.shevchenko@linux.intel.com; jarkko.nikula@linux.intel.com; > > wsa@the-dreams.de; linux-i2c@vger.kernel.org; linux- > > kernel@vger.kernel.org; Xue, Ken; Wan, Vincent; Huang, Ray; Wang, Annie; > > Li, Tony > > Subject: Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD > > controller > > > > On Thu, Nov 05, 2015 at 08:34:44PM +0800, Xiangliang Yu wrote: > > > Because of some hardware limitation, AMD I2C controller can't trigger > > > next interrupt if interrupt status has been changed after clearing > > > interrupt status bits. Then, I2C will lost interrupt and IO timeout. > > > > > > According to hardware design, this patch implements a workaround to > > > disable i2c controller interrupt after clearing interrupt bits when > > > entering ISR and to enable i2c interrupt before exiting ISR. > > > > > > To reduce the performance impacts on other vendors, use unlikely > > > function to check flag in ISR. > > > > > > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@amd.com> > > > --- > > > drivers/i2c/busses/i2c-designware-core.c | 6 ++++++ > > > drivers/i2c/busses/i2c-designware-core.h | 1 + > > > drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++++ > > > 3 files changed, 11 insertions(+) > > > > > > diff --git a/drivers/i2c/busses/i2c-designware-core.c > > > b/drivers/i2c/busses/i2c-designware-core.c > > > index 7441cdc..04e7b65 100644 > > > --- a/drivers/i2c/busses/i2c-designware-core.c > > > +++ b/drivers/i2c/busses/i2c-designware-core.c > > > @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) > > > > > > stat = i2c_dw_read_clear_intrbits(dev); > > > > What if the status changes right here, before you go and mask the interrupt? > Have no effect, because i2c controller can't trigger next interrupt. > > > > > > > > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > > > + i2c_dw_disable_int(dev); > > > + > > > if (stat & DW_IC_INTR_TX_ABRT) { > > > dev->cmd_err |= DW_IC_ERR_TX_ABRT; > > > dev->status = STATUS_IDLE; > > > @@ -811,6 +814,9 @@ tx_aborted: > > > if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || > > dev->msg_err) > > > complete(&dev->cmd_complete); > > > > > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > > > + dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, > > DW_IC_INTR_MASK); > > > > The driver disables TX interrupt when it is not needed anymore or when TX > > gets aborted but the above will re-enable all interrupts regardless. > > Is that the intention? > No, i2c controller can trigger next interrupt only after re-enable all interrupt. If you get an error the function masks all interrupts and jumps to tx_aborted label. With this patch you unmask all interrupts again before exiting the function. > > > > > + > > > return IRQ_HANDLED; > > > } > > > EXPORT_SYMBOL_GPL(i2c_dw_isr); > > > diff --git a/drivers/i2c/busses/i2c-designware-core.h > > > b/drivers/i2c/busses/i2c-designware-core.h > > > index 9630222..808ef6a 100644 > > > --- a/drivers/i2c/busses/i2c-designware-core.h > > > +++ b/drivers/i2c/busses/i2c-designware-core.h > > > @@ -111,6 +111,7 @@ struct dw_i2c_dev { > > > > > > #define ACCESS_SWAP 0x00000001 > > > #define ACCESS_16BIT 0x00000002 > > > +#define ACCESS_INTR_MASK 0x00000004 > > > > > > extern u32 dw_readl(struct dw_i2c_dev *dev, int offset); extern void > > > dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); diff --git > > > a/drivers/i2c/busses/i2c-designware-platdrv.c > > > b/drivers/i2c/busses/i2c-designware-platdrv.c > > > index 472b882..0c59357 100644 > > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > > > @@ -251,6 +251,10 @@ static int dw_i2c_probe(struct platform_device > > *pdev) > > > dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1; > > > dev->adapter.nr = pdev->id; > > > } > > > + > > > + if (!strncmp(pdev->name, "AMD0010", 7)) > > > + dev->accessor_flags |= ACCESS_INTR_MASK; > > > + > > > > Can't you put this to ->driver_data? For example it could refer to a > > configuration structure that then contains quirk flags. > I think it will need to add a function to support it, but the function > Is rarely used. It will easy to add if i2c driver has quirk mechanisms. > Added code is very simple and have no influence on others. What if the next AMD I2C controller needs another quirk, and then another? I would rather pass flags or similar (reference to config structure including flags) with ->driver_data. > > > > > > r = i2c_dw_init(dev); > > > if (r) > > > return r; > > > -- > > > 1.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller 2015-11-06 7:46 ` Mika Westerberg @ 2015-11-06 8:33 ` Yu, Xiangliang 2015-12-01 0:43 ` Wolfram Sang 0 siblings, 1 reply; 8+ messages in thread From: Yu, Xiangliang @ 2015-11-06 8:33 UTC (permalink / raw) To: Mika Westerberg Cc: andriy.shevchenko@linux.intel.com, jarkko.nikula@linux.intel.com, wsa@the-dreams.de, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Xue, Ken, Wan, Vincent, Huang, Ray, Wang, Annie, Li, Tony > -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Friday, November 06, 2015 3:46 PM > To: Yu, Xiangliang > Cc: andriy.shevchenko@linux.intel.com; jarkko.nikula@linux.intel.com; > wsa@the-dreams.de; linux-i2c@vger.kernel.org; linux- > kernel@vger.kernel.org; Xue, Ken; Wan, Vincent; Huang, Ray; Wang, Annie; > Li, Tony > Subject: Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD > controller > > On Fri, Nov 06, 2015 at 04:34:19AM +0000, Yu, Xiangliang wrote: > > > -----Original Message----- > > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > > Sent: Thursday, November 05, 2015 9:52 PM > > > To: Yu, Xiangliang > > > Cc: andriy.shevchenko@linux.intel.com; > > > jarkko.nikula@linux.intel.com; wsa@the-dreams.de; > > > linux-i2c@vger.kernel.org; linux- kernel@vger.kernel.org; Xue, Ken; > > > Wan, Vincent; Huang, Ray; Wang, Annie; Li, Tony > > > Subject: Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for > > > AMD controller > > > > > > On Thu, Nov 05, 2015 at 08:34:44PM +0800, Xiangliang Yu wrote: > > > > Because of some hardware limitation, AMD I2C controller can't > > > > trigger next interrupt if interrupt status has been changed after > > > > clearing interrupt status bits. Then, I2C will lost interrupt and IO timeout. > > > > > > > > According to hardware design, this patch implements a workaround > > > > to disable i2c controller interrupt after clearing interrupt bits > > > > when entering ISR and to enable i2c interrupt before exiting ISR. > > > > > > > > To reduce the performance impacts on other vendors, use unlikely > > > > function to check flag in ISR. > > > > > > > > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@amd.com> > > > > --- > > > > drivers/i2c/busses/i2c-designware-core.c | 6 ++++++ > > > > drivers/i2c/busses/i2c-designware-core.h | 1 + > > > > drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++++ > > > > 3 files changed, 11 insertions(+) > > > > > > > > diff --git a/drivers/i2c/busses/i2c-designware-core.c > > > > b/drivers/i2c/busses/i2c-designware-core.c > > > > index 7441cdc..04e7b65 100644 > > > > --- a/drivers/i2c/busses/i2c-designware-core.c > > > > +++ b/drivers/i2c/busses/i2c-designware-core.c > > > > @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void > > > > *dev_id) > > > > > > > > stat = i2c_dw_read_clear_intrbits(dev); > > > > > > What if the status changes right here, before you go and mask the > interrupt? > > Have no effect, because i2c controller can't trigger next interrupt. > > > > > > > > > > > > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > > > > + i2c_dw_disable_int(dev); > > > > + > > > > if (stat & DW_IC_INTR_TX_ABRT) { > > > > dev->cmd_err |= DW_IC_ERR_TX_ABRT; > > > > dev->status = STATUS_IDLE; > > > > @@ -811,6 +814,9 @@ tx_aborted: > > > > if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || > > > dev->msg_err) > > > > complete(&dev->cmd_complete); > > > > > > > > + if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) > > > > + dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, > > > DW_IC_INTR_MASK); > > > > > > The driver disables TX interrupt when it is not needed anymore or > > > when TX gets aborted but the above will re-enable all interrupts > regardless. > > > Is that the intention? > > No, i2c controller can trigger next interrupt only after re-enable all interrupt. > > If you get an error the function masks all interrupts and jumps to tx_aborted > label. With this patch you unmask all interrupts again before exiting the > function. > I see, how about change if statement to else if? > > > > > > > + > > > > return IRQ_HANDLED; > > > > } > > > > EXPORT_SYMBOL_GPL(i2c_dw_isr); > > > > diff --git a/drivers/i2c/busses/i2c-designware-core.h > > > > b/drivers/i2c/busses/i2c-designware-core.h > > > > index 9630222..808ef6a 100644 > > > > --- a/drivers/i2c/busses/i2c-designware-core.h > > > > +++ b/drivers/i2c/busses/i2c-designware-core.h > > > > @@ -111,6 +111,7 @@ struct dw_i2c_dev { > > > > > > > > #define ACCESS_SWAP 0x00000001 > > > > #define ACCESS_16BIT 0x00000002 > > > > +#define ACCESS_INTR_MASK 0x00000004 > > > > > > > > extern u32 dw_readl(struct dw_i2c_dev *dev, int offset); extern > > > > void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); diff > > > > --git a/drivers/i2c/busses/i2c-designware-platdrv.c > > > > b/drivers/i2c/busses/i2c-designware-platdrv.c > > > > index 472b882..0c59357 100644 > > > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > > > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > > > > @@ -251,6 +251,10 @@ static int dw_i2c_probe(struct > > > > platform_device > > > *pdev) > > > > dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1; > > > > dev->adapter.nr = pdev->id; > > > > } > > > > + > > > > + if (!strncmp(pdev->name, "AMD0010", 7)) > > > > + dev->accessor_flags |= ACCESS_INTR_MASK; > > > > + > > > > > > Can't you put this to ->driver_data? For example it could refer to a > > > configuration structure that then contains quirk flags. > > I think it will need to add a function to support it, but the function > > Is rarely used. It will easy to add if i2c driver has quirk mechanisms. > > Added code is very simple and have no influence on others. > > What if the next AMD I2C controller needs another quirk, and then another? > > I would rather pass flags or similar (reference to config structure including > flags) with ->driver_data. > I don't think there is another quirk. > > > > > > > > > r = i2c_dw_init(dev); > > > > if (r) > > > > return r; > > > > -- > > > > 1.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller 2015-11-06 8:33 ` Yu, Xiangliang @ 2015-12-01 0:43 ` Wolfram Sang 0 siblings, 0 replies; 8+ messages in thread From: Wolfram Sang @ 2015-12-01 0:43 UTC (permalink / raw) To: Yu, Xiangliang Cc: Mika Westerberg, andriy.shevchenko@linux.intel.com, jarkko.nikula@linux.intel.com, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Xue, Ken, Wan, Vincent, Huang, Ray, Wang, Annie, Li, Tony [-- Attachment #1: Type: text/plain, Size: 617 bytes --] > > > > The driver disables TX interrupt when it is not needed anymore or > > > > when TX gets aborted but the above will re-enable all interrupts > > regardless. > > > > Is that the intention? > > > No, i2c controller can trigger next interrupt only after re-enable all interrupt. > > > > If you get an error the function masks all interrupts and jumps to tx_aborted > > label. With this patch you unmask all interrupts again before exiting the > > function. > > > > I see, how about change if statement to else if? That sounds to me that a V2 is needed, or? Marking as "Changes requested"... [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller 2015-11-06 4:34 ` Yu, Xiangliang 2015-11-06 7:46 ` Mika Westerberg @ 2015-11-06 8:59 ` Jarkko Nikula 2015-11-06 9:11 ` Yu, Xiangliang 1 sibling, 1 reply; 8+ messages in thread From: Jarkko Nikula @ 2015-11-06 8:59 UTC (permalink / raw) To: Yu, Xiangliang, Mika Westerberg Cc: andriy.shevchenko@linux.intel.com, wsa@the-dreams.de, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Xue, Ken, Wan, Vincent, Huang, Ray, Wang, Annie, Li, Tony On 06.11.2015 06:34, Yu, Xiangliang wrote: >> -----Original Message----- >> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] >>> --- a/drivers/i2c/busses/i2c-designware-core.c >>> +++ b/drivers/i2c/busses/i2c-designware-core.c >>> @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) >>> >>> stat = i2c_dw_read_clear_intrbits(dev); >> >> What if the status changes right here, before you go and mask the interrupt? > Have no effect, because i2c controller can't trigger next interrupt. > Does it mean possible lost interrupt too? I guess it can be debugged by placing a few ms long mdelay() between clearing and masking. How frequent is this timeout? I guess lost interrupt is somehow nearly related to clearing, masking and unmasking if that cycle helps. One thing that comes to my mind is masking needed and could plain unmasking be also a working workaround? Have you tried to print DW_IC_INTR_STAT, DW_IC_INTR_MASK and DW_IC_RAW_INTR_STAT when timeout occurs? E.g. just after printing the timeout out error in i2c_dw_xfer(). Probably good in i2c_dw_isr() too if if doesn't produce too much data and make debugging impossible. -- Jarkko ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller 2015-11-06 8:59 ` Jarkko Nikula @ 2015-11-06 9:11 ` Yu, Xiangliang 0 siblings, 0 replies; 8+ messages in thread From: Yu, Xiangliang @ 2015-11-06 9:11 UTC (permalink / raw) To: Jarkko Nikula, Mika Westerberg Cc: andriy.shevchenko@linux.intel.com, wsa@the-dreams.de, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Xue, Ken, Wan, Vincent, Huang, Ray, Wang, Annie, Li, Tony > -----Original Message----- > From: Jarkko Nikula [mailto:jarkko.nikula@linux.intel.com] > Sent: Friday, November 06, 2015 5:00 PM > To: Yu, Xiangliang; Mika Westerberg > Cc: andriy.shevchenko@linux.intel.com; wsa@the-dreams.de; linux- > i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Xue, Ken; Wan, Vincent; > Huang, Ray; Wang, Annie; Li, Tony > Subject: Re: [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD > controller > > On 06.11.2015 06:34, Yu, Xiangliang wrote: > >> -----Original Message----- > >> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > >>> --- a/drivers/i2c/busses/i2c-designware-core.c > >>> +++ b/drivers/i2c/busses/i2c-designware-core.c > >>> @@ -783,6 +783,9 @@ irqreturn_t i2c_dw_isr(int this_irq, void > >>> *dev_id) > >>> > >>> stat = i2c_dw_read_clear_intrbits(dev); > >> > >> What if the status changes right here, before you go and mask the > interrupt? > > Have no effect, because i2c controller can't trigger next interrupt. > > > Does it mean possible lost interrupt too? I guess it can be debugged by > placing a few ms long mdelay() between clearing and masking. > The interrupt will not be lost if status bits change before masking interrupt. > How frequent is this timeout? I guess lost interrupt is somehow nearly > related to clearing, masking and unmasking if that cycle helps. One thing that > comes to my mind is masking needed and could plain unmasking be also a > working workaround? Quite frequently. > > Have you tried to print DW_IC_INTR_STAT, DW_IC_INTR_MASK and > DW_IC_RAW_INTR_STAT when timeout occurs? E.g. just after printing the > timeout out error in i2c_dw_xfer(). Probably good in i2c_dw_isr() too if if > doesn't produce too much data and make debugging impossible. > Yes, I have tried to print these status. I have confirmed the issue with hardware engineer. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-12-01 0:43 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-05 12:34 [PATCH 1/1] I2C: designware: fix IO timeout issue for AMD controller Xiangliang Yu 2015-11-05 13:52 ` Mika Westerberg 2015-11-06 4:34 ` Yu, Xiangliang 2015-11-06 7:46 ` Mika Westerberg 2015-11-06 8:33 ` Yu, Xiangliang 2015-12-01 0:43 ` Wolfram Sang 2015-11-06 8:59 ` Jarkko Nikula 2015-11-06 9:11 ` Yu, Xiangliang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).