[v2,1/2] i2c: designware: fix poll-after-enable regression

Not all revisions of DW I2C controller implement the enable status register.
On platforms where that's the case (e.g. BG2CD and SPEAr ARM SoCs), waiting
for enable will time out as reading the unimplemented register yields zero.

It was observed that reading the IC_ENABLE_STATUS register once suffices to
avoid getting it stuck on Bay Trail hardware, so replace polling with one
dummy read of the register.

Cc: Ben Gardner <gardner.ben@gmail.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Fixes: fba4adbbf670 ("i2c: designware: must wait for enable")
Tested-by: Ben Gardner <gardner.ben@gmail.com>
Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
---
 drivers/i2c/busses/i2c-designware-master.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On Sat, Apr 28, 2018 at 8:56 AM, Alexander Monakov <amonakov@ispras.ru> wrote:
> Not all revisions of DW I2C controller implement the enable status register.
> On platforms where that's the case (e.g. BG2CD and SPEAr ARM SoCs), waiting
> for enable will time out as reading the unimplemented register yields zero.
>
> It was observed that reading the IC_ENABLE_STATUS register once suffices to
> avoid getting it stuck on Bay Trail hardware, so replace polling with one
> dummy read of the register.
>
> Cc: Ben Gardner <gardner.ben@gmail.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Fixes: fba4adbbf670 ("i2c: designware: must wait for enable")
> Tested-by: Ben Gardner <gardner.ben@gmail.com>
> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> ---
>  drivers/i2c/busses/i2c-designware-master.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index fd36c39ddf4e..0cdba29ae0a9 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -209,7 +209,10 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>         i2c_dw_disable_int(dev);
>
>         /* Enable the adapter */
> -       __i2c_dw_enable_and_wait(dev, true);
> +       __i2c_dw_enable(dev, true);
> +
> +       /* Dummy read to avoid the register getting stuck on Bay Trail */
> +       dw_readl(dev, DW_IC_ENABLE_STATUS);
>
>         /* Clear and enable interrupts */
>         dw_readl(dev, DW_IC_CLR_INTR);
> --
Tested-by: Ben Gardner <gardner.ben@gmail.com>

Thanks.

On 04/30/2018 07:08 PM, Ben Gardner wrote:
> On Sat, Apr 28, 2018 at 8:56 AM, Alexander Monakov <amonakov@ispras.ru> wrote:
>> Not all revisions of DW I2C controller implement the enable status register.
>> On platforms where that's the case (e.g. BG2CD and SPEAr ARM SoCs), waiting
>> for enable will time out as reading the unimplemented register yields zero.
>>
>> It was observed that reading the IC_ENABLE_STATUS register once suffices to
>> avoid getting it stuck on Bay Trail hardware, so replace polling with one
>> dummy read of the register.
>>
>> Cc: Ben Gardner <gardner.ben@gmail.com>
>> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Fixes: fba4adbbf670 ("i2c: designware: must wait for enable")
>> Tested-by: Ben Gardner <gardner.ben@gmail.com>
>> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
>> ---
>>   drivers/i2c/busses/i2c-designware-master.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
>> index fd36c39ddf4e..0cdba29ae0a9 100644
>> --- a/drivers/i2c/busses/i2c-designware-master.c
>> +++ b/drivers/i2c/busses/i2c-designware-master.c
>> @@ -209,7 +209,10 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>>          i2c_dw_disable_int(dev);
>>
>>          /* Enable the adapter */
>> -       __i2c_dw_enable_and_wait(dev, true);
>> +       __i2c_dw_enable(dev, true);
>> +
>> +       /* Dummy read to avoid the register getting stuck on Bay Trail */
>> +       dw_readl(dev, DW_IC_ENABLE_STATUS);
>>
>>          /* Clear and enable interrupts */
>>          dw_readl(dev, DW_IC_CLR_INTR);
>> --
> Tested-by: Ben Gardner <gardner.ben@gmail.com>
> 
I noticed the loop in __i2c_dw_enable_and_wait() iterated twice on a 
Baytrail I have when enabling. On a few other machines only once. I 
guess that's the reason why above dummy read works here. Hopefully it 
doesn't need longer setup time on some platform.

I also debugged does IC_STATUS and IC_RAW_INTR_STAT show similar delay 
but they turned instantly so I guess they cannot be used as an 
alternative for DW_IC_ENABLE_STATUS polling directly for all platforms.

So I think we could try this patch since it fixes the issue and we don't 
know what platforms implement enable status in IC_ENABLE_STATUS.

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

On Sat, Apr 28, 2018 at 04:56:06PM +0300, Alexander Monakov wrote:
> Not all revisions of DW I2C controller implement the enable status register.
> On platforms where that's the case (e.g. BG2CD and SPEAr ARM SoCs), waiting
> for enable will time out as reading the unimplemented register yields zero.
> 
> It was observed that reading the IC_ENABLE_STATUS register once suffices to
> avoid getting it stuck on Bay Trail hardware, so replace polling with one
> dummy read of the register.
> 
> Cc: Ben Gardner <gardner.ben@gmail.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Fixes: fba4adbbf670 ("i2c: designware: must wait for enable")
> Tested-by: Ben Gardner <gardner.ben@gmail.com>
> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>

Applied to for-current, thanks!