[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Gregory CLEMENT]

Offload can be used only on regular transactions and for 1 to byte
transfers. In the other cases we switch back to usual work flow.

In this case we need to call mv64xxx_i2c_prepare_for_io() as this
function is not used when we try to use offloading.

This commit adds this missing call when offloading have failed in the
MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.

This fix the timeout seen when the the i2c driver try to access an
address where the device is absent on the Armada XP bases board.

Cc: stable@vger.kernel.org # v3.12+
Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b8c5187b9ee0..a1700c62d955 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
 		if (!mv64xxx_i2c_offload_msg(drv_data))
 			break;
-		else
+		else {
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+			/*
+			 * Switch to the standard path, so we finally need to
+			 * prepare the io that have not been done in
+			 * mv64xxx_i2c_execute_msg
+			 */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-- 
1.8.1.2

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Gregory CLEMENT]

Offload can be used only on regular transactions and for 1 to byte
transfers. In the other cases we switch back to usual work flow.

In this case we need to call mv64xxx_i2c_prepare_for_io() as this
function is not used when we try to use offloading.

This commit adds this missing call when offloading have failed in the
MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.

This fix the timeout seen when the the i2c driver try to access an
address where the device is absent on the Armada XP bases board.

Cc: stable at vger.kernel.org # v3.12+
Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b8c5187b9ee0..a1700c62d955 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
 		if (!mv64xxx_i2c_offload_msg(drv_data))
 			break;
-		else
+		else {
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+			/*
+			 * Switch to the standard path, so we finally need to
+			 * prepare the io that have not been done in
+			 * mv64xxx_i2c_execute_msg
+			 */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-- 
1.8.1.2

Re: [PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Jason Cooper]

On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
> Offload can be used only on regular transactions and for 1 to byte
> transfers. In the other cases we switch back to usual work flow.
> 
> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
> function is not used when we try to use offloading.
> 
> This commit adds this missing call when offloading have failed in the
> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
> 
> This fix the timeout seen when the the i2c driver try to access an
> address where the device is absent on the Armada XP bases board.
> 
> Cc: stable@vger.kernel.org # v3.12+
> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

I'd like to get a few tested-by's on this before this is pushed.  We've
had quite a bit of fixes this round :(  Please test both multi_v7 and
mvebu defconfigs.

Kevin, I know you're busy with a lot more than us, but if you could
confirm that this fixes the bus hangs you reported, that would be great.

thx,

Jason.

> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index b8c5187b9ee0..a1700c62d955 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>  			break;
> -		else
> +		else {
>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +			/*
> +			 * Switch to the standard path, so we finally need to
> +			 * prepare the io that have not been done in
> +			 * mv64xxx_i2c_execute_msg
> +			 */
> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +		}
>  		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -- 
> 1.8.1.2
> 

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Jason Cooper]

On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
> Offload can be used only on regular transactions and for 1 to byte
> transfers. In the other cases we switch back to usual work flow.
> 
> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
> function is not used when we try to use offloading.
> 
> This commit adds this missing call when offloading have failed in the
> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
> 
> This fix the timeout seen when the the i2c driver try to access an
> address where the device is absent on the Armada XP bases board.
> 
> Cc: stable at vger.kernel.org # v3.12+
> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

I'd like to get a few tested-by's on this before this is pushed.  We've
had quite a bit of fixes this round :(  Please test both multi_v7 and
mvebu defconfigs.

Kevin, I know you're busy with a lot more than us, but if you could
confirm that this fixes the bus hangs you reported, that would be great.

thx,

Jason.

> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index b8c5187b9ee0..a1700c62d955 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>  			break;
> -		else
> +		else {
>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +			/*
> +			 * Switch to the standard path, so we finally need to
> +			 * prepare the io that have not been done in
> +			 * mv64xxx_i2c_execute_msg
> +			 */
> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +		}
>  		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -- 
> 1.8.1.2
> 

Re: [PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Gregory CLEMENT]

On 07/02/2014 16:09, Jason Cooper wrote:
> On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
>> Offload can be used only on regular transactions and for 1 to byte
>> transfers. In the other cases we switch back to usual work flow.
>>
>> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
>> function is not used when we try to use offloading.
>>
>> This commit adds this missing call when offloading have failed in the
>> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
>>
>> This fix the timeout seen when the the i2c driver try to access an
>> address where the device is absent on the Armada XP bases board.
>>
>> Cc: stable@vger.kernel.org # v3.12+
>> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> I'd like to get a few tested-by's on this before this is pushed.  We've
> had quite a bit of fixes this round :(  Please test both multi_v7 and
> mvebu defconfigs.

Well of course I have tested with multi_v7 and mvebu defconfigs, but I
think it is not my tested-by that you want! :)

An for the test we only have to test the Armada XP B0 based board, because
in the other case the offload is disable, so the code I modified never
doesn't run.


Thanks,

Gregory


> 
> Kevin, I know you're busy with a lot more than us, but if you could
> confirm that this fixes the bus hangs you reported, that would be great.
> 
> thx,
> 
> Jason.
> 
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index b8c5187b9ee0..a1700c62d955 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>>  			break;
>> -		else
>> +		else {
>>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
>> +			/*
>> +			 * Switch to the standard path, so we finally need to
>> +			 * prepare the io that have not been done in
>> +			 * mv64xxx_i2c_execute_msg
>> +			 */
>> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
>> +		}
>>  		/* FALLTHRU */
>>  	case MV64XXX_I2C_ACTION_SEND_START:
>>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>> -- 
>> 1.8.1.2
>>


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Gregory CLEMENT]

On 07/02/2014 16:09, Jason Cooper wrote:
> On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
>> Offload can be used only on regular transactions and for 1 to byte
>> transfers. In the other cases we switch back to usual work flow.
>>
>> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
>> function is not used when we try to use offloading.
>>
>> This commit adds this missing call when offloading have failed in the
>> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
>>
>> This fix the timeout seen when the the i2c driver try to access an
>> address where the device is absent on the Armada XP bases board.
>>
>> Cc: stable at vger.kernel.org # v3.12+
>> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> I'd like to get a few tested-by's on this before this is pushed.  We've
> had quite a bit of fixes this round :(  Please test both multi_v7 and
> mvebu defconfigs.

Well of course I have tested with multi_v7 and mvebu defconfigs, but I
think it is not my tested-by that you want! :)

An for the test we only have to test the Armada XP B0 based board, because
in the other case the offload is disable, so the code I modified never
doesn't run.


Thanks,

Gregory


> 
> Kevin, I know you're busy with a lot more than us, but if you could
> confirm that this fixes the bus hangs you reported, that would be great.
> 
> thx,
> 
> Jason.
> 
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index b8c5187b9ee0..a1700c62d955 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>>  			break;
>> -		else
>> +		else {
>>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
>> +			/*
>> +			 * Switch to the standard path, so we finally need to
>> +			 * prepare the io that have not been done in
>> +			 * mv64xxx_i2c_execute_msg
>> +			 */
>> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
>> +		}
>>  		/* FALLTHRU */
>>  	case MV64XXX_I2C_ACTION_SEND_START:
>>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>> -- 
>> 1.8.1.2
>>


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Kevin Hilman]

Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> writes:

> On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
>> Offload can be used only on regular transactions and for 1 to byte
>> transfers. In the other cases we switch back to usual work flow.
>> 
>> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
>> function is not used when we try to use offloading.
>> 
>> This commit adds this missing call when offloading have failed in the
>> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
>> 
>> This fix the timeout seen when the the i2c driver try to access an
>> address where the device is absent on the Armada XP bases board.
>> 
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v3.12+
>> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> I'd like to get a few tested-by's on this before this is pushed.  We've
> had quite a bit of fixes this round :(  Please test both multi_v7 and
> mvebu defconfigs.
>
> Kevin, I know you're busy with a lot more than us, but if you could
> confirm that this fixes the bus hangs you reported, that would be great.

I applied this patch on top of next-20140207 and tested this on the
armada-xp openblocks ax3, which is where I was seeing the I2C timeouts.

I confirm it fixes the timeouts I was seeing.

Tested-by: Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Kevin

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Kevin Hilman]

Jason Cooper <jason@lakedaemon.net> writes:

> On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
>> Offload can be used only on regular transactions and for 1 to byte
>> transfers. In the other cases we switch back to usual work flow.
>> 
>> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
>> function is not used when we try to use offloading.
>> 
>> This commit adds this missing call when offloading have failed in the
>> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
>> 
>> This fix the timeout seen when the the i2c driver try to access an
>> address where the device is absent on the Armada XP bases board.
>> 
>> Cc: stable at vger.kernel.org # v3.12+
>> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> I'd like to get a few tested-by's on this before this is pushed.  We've
> had quite a bit of fixes this round :(  Please test both multi_v7 and
> mvebu defconfigs.
>
> Kevin, I know you're busy with a lot more than us, but if you could
> confirm that this fixes the bus hangs you reported, that would be great.

I applied this patch on top of next-20140207 and tested this on the
armada-xp openblocks ax3, which is where I was seeing the I2C timeouts.

I confirm it fixes the timeouts I was seeing.

Tested-by: Kevin Hilman <khilman@linaro.org>

Kevin

Re: [PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Jason Cooper]

On Fri, Feb 07, 2014 at 10:09:53AM -0800, Kevin Hilman wrote:
> Jason Cooper <jason@lakedaemon.net> writes:
> 
> > On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
> >> Offload can be used only on regular transactions and for 1 to byte
> >> transfers. In the other cases we switch back to usual work flow.
> >> 
> >> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
> >> function is not used when we try to use offloading.
> >> 
> >> This commit adds this missing call when offloading have failed in the
> >> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
> >> 
> >> This fix the timeout seen when the the i2c driver try to access an
> >> address where the device is absent on the Armada XP bases board.
> >> 
> >> Cc: stable@vger.kernel.org # v3.12+
> >> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
> >> 
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >> ---
> >>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > I'd like to get a few tested-by's on this before this is pushed.  We've
> > had quite a bit of fixes this round :(  Please test both multi_v7 and
> > mvebu defconfigs.
> >
> > Kevin, I know you're busy with a lot more than us, but if you could
> > confirm that this fixes the bus hangs you reported, that would be great.
> 
> I applied this patch on top of next-20140207 and tested this on the
> armada-xp openblocks ax3, which is where I was seeing the I2C timeouts.
> 
> I confirm it fixes the timeouts I was seeing.
> 
> Tested-by: Kevin Hilman <khilman@linaro.org>

Thanks, Kevin!

thx,

Jason.

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Jason Cooper]

On Fri, Feb 07, 2014 at 10:09:53AM -0800, Kevin Hilman wrote:
> Jason Cooper <jason@lakedaemon.net> writes:
> 
> > On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
> >> Offload can be used only on regular transactions and for 1 to byte
> >> transfers. In the other cases we switch back to usual work flow.
> >> 
> >> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
> >> function is not used when we try to use offloading.
> >> 
> >> This commit adds this missing call when offloading have failed in the
> >> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
> >> 
> >> This fix the timeout seen when the the i2c driver try to access an
> >> address where the device is absent on the Armada XP bases board.
> >> 
> >> Cc: stable at vger.kernel.org # v3.12+
> >> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
> >> 
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >> ---
> >>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > I'd like to get a few tested-by's on this before this is pushed.  We've
> > had quite a bit of fixes this round :(  Please test both multi_v7 and
> > mvebu defconfigs.
> >
> > Kevin, I know you're busy with a lot more than us, but if you could
> > confirm that this fixes the bus hangs you reported, that would be great.
> 
> I applied this patch on top of next-20140207 and tested this on the
> armada-xp openblocks ax3, which is where I was seeing the I2C timeouts.
> 
> I confirm it fixes the timeouts I was seeing.
> 
> Tested-by: Kevin Hilman <khilman@linaro.org>

Thanks, Kevin!

thx,

Jason.

Re: [PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Jason Cooper]

On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
> Offload can be used only on regular transactions and for 1 to byte
> transfers. In the other cases we switch back to usual work flow.
> 
> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
> function is not used when we try to use offloading.
> 
> This commit adds this missing call when offloading have failed in the
> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
> 
> This fix the timeout seen when the the i2c driver try to access an
> address where the device is absent on the Armada XP bases board.
> 
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v3.12+
> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Acked-by: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>

thx,

Jason.

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Jason Cooper]

On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote:
> Offload can be used only on regular transactions and for 1 to byte
> transfers. In the other cases we switch back to usual work flow.
> 
> In this case we need to call mv64xxx_i2c_prepare_for_io() as this
> function is not used when we try to use offloading.
> 
> This commit adds this missing call when offloading have failed in the
> MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case.
> 
> This fix the timeout seen when the the i2c driver try to access an
> address where the device is absent on the Armada XP bases board.
> 
> Cc: stable at vger.kernel.org # v3.12+
> Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support)
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

Re: [PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>  			break;
> -		else
> +		else {

Here you break the coding style...

>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +			/*
> +			 * Switch to the standard path, so we finally need to
> +			 * prepare the io that have not been done in
> +			 * mv64xxx_i2c_execute_msg
> +			 */
> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +		}

... but more importantly, NACK. The code is already hard to follow which
is the cause for this bug. This snipplet makes the code even harder to
read, so it needs some simplification IMO. I'll fire up a counterpatch
in a minute to explain what I mean.

>  		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -- 
> 1.8.1.2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Wolfram Sang]

> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>  			break;
> -		else
> +		else {

Here you break the coding style...

>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +			/*
> +			 * Switch to the standard path, so we finally need to
> +			 * prepare the io that have not been done in
> +			 * mv64xxx_i2c_execute_msg
> +			 */
> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +		}

... but more importantly, NACK. The code is already hard to follow which
is the cause for this bug. This snipplet makes the code even harder to
read, so it needs some simplification IMO. I'll fire up a counterpatch
in a minute to explain what I mean.

>  		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -- 
> 1.8.1.2
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140213/cee40674/attachment.sig>

Re: [PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Gregory CLEMENT]

Hi Wolfram,

Thanks for you review,


On 13/02/2014 10:41, Wolfram Sang wrote:
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>>  			break;
>> -		else
>> +		else {
> 
> Here you break the coding style...
> 
>>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
>> +			/*
>> +			 * Switch to the standard path, so we finally need to
>> +			 * prepare the io that have not been done in
>> +			 * mv64xxx_i2c_execute_msg
>> +			 */
>> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
>> +		}
> 
> ... but more importantly, NACK. The code is already hard to follow which
> is the cause for this bug. This snipplet makes the code even harder to
> read, so it needs some simplification IMO. I'll fire up a counterpatch
> in a minute to explain what I mean.

OK, as long as we have a fix for this issue.

I am waiting for your patch.

Thanks,

Gregory



> 
>>  		/* FALLTHRU */
>>  	case MV64XXX_I2C_ACTION_SEND_START:
>>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>> -- 
>> 1.8.1.2
>>


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] i2c: mv64xxx: Fix locked bus when offload is selected but not used on a message

[Gregory CLEMENT]

Hi Wolfram,

Thanks for you review,


On 13/02/2014 10:41, Wolfram Sang wrote:
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -461,8 +461,15 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>>  	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
>>  		if (!mv64xxx_i2c_offload_msg(drv_data))
>>  			break;
>> -		else
>> +		else {
> 
> Here you break the coding style...
> 
>>  			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
>> +			/*
>> +			 * Switch to the standard path, so we finally need to
>> +			 * prepare the io that have not been done in
>> +			 * mv64xxx_i2c_execute_msg
>> +			 */
>> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
>> +		}
> 
> ... but more importantly, NACK. The code is already hard to follow which
> is the cause for this bug. This snipplet makes the code even harder to
> read, so it needs some simplification IMO. I'll fire up a counterpatch
> in a minute to explain what I mean.

OK, as long as we have a fix for this issue.

I am waiting for your patch.

Thanks,

Gregory



> 
>>  		/* FALLTHRU */
>>  	case MV64XXX_I2C_ACTION_SEND_START:
>>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
>> -- 
>> 1.8.1.2
>>


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/5] mv64xxx updates

[Wolfram Sang]

So, this is a series I came up with trying to fix the issue found by Kevin.
Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
are further cleanup possibilities. And there is still more potential, I mainly
wanted to give some inspiration and awareness that the driver could need some
more love. Please test at least 1+2, comments to 3-5 very welcome.

Sorry for the delay, I got distracted by an NMI.

Wolfram Sang (5):
  i2c: mv64xxx: put offload check into offload prepare function
  i2c: mv64xxx: refactor message start to ensure proper initialization
  i2c: mv64xxx: refactor send_start
  i2c: mv64xxx: directly call send_start when initializing transfer
  i2c: mv64xxx: refactor initialization for new msgs

 drivers/i2c/busses/i2c-mv64xxx.c | 67 ++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 40 deletions(-)

-- 
1.8.5.1

[PATCH 0/5] mv64xxx updates

[Wolfram Sang]

So, this is a series I came up with trying to fix the issue found by Kevin.
Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
are further cleanup possibilities. And there is still more potential, I mainly
wanted to give some inspiration and awareness that the driver could need some
more love. Please test at least 1+2, comments to 3-5 very welcome.

Sorry for the delay, I got distracted by an NMI.

Wolfram Sang (5):
  i2c: mv64xxx: put offload check into offload prepare function
  i2c: mv64xxx: refactor message start to ensure proper initialization
  i2c: mv64xxx: refactor send_start
  i2c: mv64xxx: directly call send_start when initializing transfer
  i2c: mv64xxx: refactor initialization for new msgs

 drivers/i2c/busses/i2c-mv64xxx.c | 67 ++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 40 deletions(-)

-- 
1.8.5.1

[PATCH 1/5] i2c: mv64xxx: put offload check into offload prepare function

[Wolfram Sang]

It makes code simpler to read and prepares a reorganization needed for a
bugfix.

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b8c5187..ba64978 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -204,6 +204,9 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
 	unsigned long ctrl_reg;
 	struct i2c_msg *msg = drv_data->msgs;
 
+	if (!drv_data->offload_enabled)
+		return -EOPNOTSUPP;
+
 	drv_data->msg = msg;
 	drv_data->byte_posn = 0;
 	drv_data->bytes_left = msg->len;
@@ -433,8 +436,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (!(drv_data->offload_enabled &&
-				mv64xxx_i2c_offload_msg(drv_data))) {
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
 			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
 			writel(drv_data->cntl_bits,
 			drv_data->reg_base + drv_data->reg_offsets.control);
-- 
1.8.5.1

[PATCH 1/5] i2c: mv64xxx: put offload check into offload prepare function

[Wolfram Sang]

It makes code simpler to read and prepares a reorganization needed for a
bugfix.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b8c5187..ba64978 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -204,6 +204,9 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
 	unsigned long ctrl_reg;
 	struct i2c_msg *msg = drv_data->msgs;
 
+	if (!drv_data->offload_enabled)
+		return -EOPNOTSUPP;
+
 	drv_data->msg = msg;
 	drv_data->byte_posn = 0;
 	drv_data->bytes_left = msg->len;
@@ -433,8 +436,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (!(drv_data->offload_enabled &&
-				mv64xxx_i2c_offload_msg(drv_data))) {
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
 			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
 			writel(drv_data->cntl_bits,
 			drv_data->reg_base + drv_data->reg_offsets.control);
-- 
1.8.5.1

[PATCH 2/5] i2c: mv64xxx: refactor message start to ensure proper initialization

[Wolfram Sang]

Because the offload mechanism can fall back to a standard transfer,
having two seperate initialization states is unfortunate. Let's just
have one state which does things consistently. This fixes a bug where
some preparation was missing when the fallback happened. And it makes
the code much easier to follow.

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index ba64978..d52d849 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -97,7 +97,6 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
 	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
@@ -460,15 +459,14 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
-	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
-		if (!mv64xxx_i2c_offload_msg(drv_data))
-			break;
-		else
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
-		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		/* Can we offload this msg ? */
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+			/* No, switch to standard path */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+				drv_data->reg_base + drv_data->reg_offsets.control);
+		}
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
@@ -627,15 +625,10 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	if (drv_data->offload_enabled) {
-		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	} else {
-		mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	}
+	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
 	mv64xxx_i2c_do_action(drv_data);
-- 
1.8.5.1

[PATCH 2/5] i2c: mv64xxx: refactor message start to ensure proper initialization

[Wolfram Sang]

Because the offload mechanism can fall back to a standard transfer,
having two seperate initialization states is unfortunate. Let's just
have one state which does things consistently. This fixes a bug where
some preparation was missing when the fallback happened. And it makes
the code much easier to follow.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index ba64978..d52d849 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -97,7 +97,6 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
 	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
@@ -460,15 +459,14 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
-	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
-		if (!mv64xxx_i2c_offload_msg(drv_data))
-			break;
-		else
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
-		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		/* Can we offload this msg ? */
+		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+			/* No, switch to standard path */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+				drv_data->reg_base + drv_data->reg_offsets.control);
+		}
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
@@ -627,15 +625,10 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	if (drv_data->offload_enabled) {
-		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	} else {
-		mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
-	}
+	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
 	mv64xxx_i2c_do_action(drv_data);
-- 
1.8.5.1

[PATCH 3/5] i2c: mv64xxx: refactor send_start

[Wolfram Sang]

For start and restart, we are doing the same thing. Let's consolidate
that.

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index d52d849..9c37b59 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -419,6 +419,17 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 	}
 }
 
+static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
+{
+	/* Can we offload this msg ? */
+	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+		/* No, switch to standard path */
+		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+			drv_data->reg_base + drv_data->reg_offsets.control);
+	}
+}
+
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {
@@ -435,14 +446,11 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-			writel(drv_data->cntl_bits,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		// CHECKME: Does it work? Order of writel and prepare_for_io is
+		// exchanged. Also, do we need to change cntl_bits in drv_data
+		// with |= MV64XXX_I2C_REG_CONTROL_START?
+		mv64xxx_i2c_send_start(drv_data);
 
-			/* Setup for the next message */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-		}
 		if (drv_data->errata_delay)
 			udelay(5);
 
@@ -460,13 +468,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_START:
-		/* Can we offload this msg ? */
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			/* No, switch to standard path */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-				drv_data->reg_base + drv_data->reg_offsets.control);
-		}
+		mv64xxx_i2c_send_start(drv_data);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
-- 
1.8.5.1

[PATCH 3/5] i2c: mv64xxx: refactor send_start

[Wolfram Sang]

For start and restart, we are doing the same thing. Let's consolidate
that.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index d52d849..9c37b59 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -419,6 +419,17 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 	}
 }
 
+static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
+{
+	/* Can we offload this msg ? */
+	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
+		/* No, switch to standard path */
+		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
+			drv_data->reg_base + drv_data->reg_offsets.control);
+	}
+}
+
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {
@@ -435,14 +446,11 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-			writel(drv_data->cntl_bits,
-			drv_data->reg_base + drv_data->reg_offsets.control);
+		// CHECKME: Does it work? Order of writel and prepare_for_io is
+		// exchanged. Also, do we need to change cntl_bits in drv_data
+		// with |= MV64XXX_I2C_REG_CONTROL_START?
+		mv64xxx_i2c_send_start(drv_data);
 
-			/* Setup for the next message */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-		}
 		if (drv_data->errata_delay)
 			udelay(5);
 
@@ -460,13 +468,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_START:
-		/* Can we offload this msg ? */
-		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
-			/* No, switch to standard path */
-			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-				drv_data->reg_base + drv_data->reg_offsets.control);
-		}
+		mv64xxx_i2c_send_start(drv_data);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
-- 
1.8.5.1

[PATCH 4/5] i2c: mv64xxx: directly call send_start when initializing transfer

[Wolfram Sang]

Calling the state machine with a definite state which is only used in
this context is superfluous. Do it directly.

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 9c37b59..3af5266 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -97,7 +97,6 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
 	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
 	MV64XXX_I2C_ACTION_SEND_ADDR_1,
@@ -467,10 +466,6 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
-	case MV64XXX_I2C_ACTION_SEND_START:
-		mv64xxx_i2c_send_start(drv_data);
-		break;
-
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
 		writel(drv_data->addr1,
 			drv_data->reg_base + drv_data->reg_offsets.data);
@@ -628,12 +623,11 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 
 	spin_lock_irqsave(&drv_data->lock, flags);
 
-	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
 	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
 
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
-	mv64xxx_i2c_do_action(drv_data);
+	mv64xxx_i2c_send_start(drv_data);
 	spin_unlock_irqrestore(&drv_data->lock, flags);
 
 	mv64xxx_i2c_wait_for_completion(drv_data);
-- 
1.8.5.1

[PATCH 4/5] i2c: mv64xxx: directly call send_start when initializing transfer

[Wolfram Sang]

Calling the state machine with a definite state which is only used in
this context is superfluous. Do it directly.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 9c37b59..3af5266 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -97,7 +97,6 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
-	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
 	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
 	MV64XXX_I2C_ACTION_SEND_ADDR_1,
@@ -467,10 +466,6 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
-	case MV64XXX_I2C_ACTION_SEND_START:
-		mv64xxx_i2c_send_start(drv_data);
-		break;
-
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
 		writel(drv_data->addr1,
 			drv_data->reg_base + drv_data->reg_offsets.data);
@@ -628,12 +623,11 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 
 	spin_lock_irqsave(&drv_data->lock, flags);
 
-	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
 	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
 
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
-	mv64xxx_i2c_do_action(drv_data);
+	mv64xxx_i2c_send_start(drv_data);
 	spin_unlock_irqrestore(&drv_data->lock, flags);
 
 	mv64xxx_i2c_wait_for_completion(drv_data);
-- 
1.8.5.1

[PATCH 5/5] i2c: mv64xxx: refactor initialization for new msgs

[Wolfram Sang]

We now have a central place to put this code to.

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 3af5266..be4b0a1 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -175,11 +175,6 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 {
 	u32	dir = 0;
 
-	drv_data->msg = msg;
-	drv_data->byte_posn = 0;
-	drv_data->bytes_left = msg->len;
-	drv_data->aborting = 0;
-	drv_data->rc = 0;
 	drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK |
 		MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN;
 
@@ -205,11 +200,6 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
 	if (!drv_data->offload_enabled)
 		return -EOPNOTSUPP;
 
-	drv_data->msg = msg;
-	drv_data->byte_posn = 0;
-	drv_data->bytes_left = msg->len;
-	drv_data->aborting = 0;
-	drv_data->rc = 0;
 	/* Only regular transactions can be offloaded */
 	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
 		return -EINVAL;
@@ -420,6 +410,12 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 
 static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
 {
+	drv_data->msg = drv_data->msgs;
+	drv_data->byte_posn = 0;
+	drv_data->bytes_left = drv_data->msg->len;
+	drv_data->aborting = 0;
+	drv_data->rc = 0;
+
 	/* Can we offload this msg ? */
 	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
 		/* No, switch to standard path */
-- 
1.8.5.1

[PATCH 5/5] i2c: mv64xxx: refactor initialization for new msgs

[Wolfram Sang]

We now have a central place to put this code to.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 3af5266..be4b0a1 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -175,11 +175,6 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 {
 	u32	dir = 0;
 
-	drv_data->msg = msg;
-	drv_data->byte_posn = 0;
-	drv_data->bytes_left = msg->len;
-	drv_data->aborting = 0;
-	drv_data->rc = 0;
 	drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK |
 		MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN;
 
@@ -205,11 +200,6 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
 	if (!drv_data->offload_enabled)
 		return -EOPNOTSUPP;
 
-	drv_data->msg = msg;
-	drv_data->byte_posn = 0;
-	drv_data->bytes_left = msg->len;
-	drv_data->aborting = 0;
-	drv_data->rc = 0;
 	/* Only regular transactions can be offloaded */
 	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
 		return -EINVAL;
@@ -420,6 +410,12 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 
 static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
 {
+	drv_data->msg = drv_data->msgs;
+	drv_data->byte_posn = 0;
+	drv_data->bytes_left = drv_data->msg->len;
+	drv_data->aborting = 0;
+	drv_data->rc = 0;
+
 	/* Can we offload this msg ? */
 	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
 		/* No, switch to standard path */
-- 
1.8.5.1

Re: [PATCH 0/5] mv64xxx updates

[Gregory CLEMENT]

Hi Wolfram,

On 13/02/2014 21:36, Wolfram Sang wrote:
> So, this is a series I came up with trying to fix the issue found by Kevin.
> Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
> Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
> are further cleanup possibilities. And there is still more potential, I mainly
> wanted to give some inspiration and awareness that the driver could need some
> more love. Please test at least 1+2, comments to 3-5 very welcome.
> 
> Sorry for the delay, I got distracted by an NMI.
> 

Thanks for this series, indeed the code looks better.

I will test it tomorrow and let you know if it fixed the bug.
I will also take time to review the RFC patches.

Greogry


> Wolfram Sang (5):
>   i2c: mv64xxx: put offload check into offload prepare function
>   i2c: mv64xxx: refactor message start to ensure proper initialization
>   i2c: mv64xxx: refactor send_start
>   i2c: mv64xxx: directly call send_start when initializing transfer
>   i2c: mv64xxx: refactor initialization for new msgs
> 
>  drivers/i2c/busses/i2c-mv64xxx.c | 67 ++++++++++++++++------------------------
>  1 file changed, 27 insertions(+), 40 deletions(-)
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/5] mv64xxx updates

[Gregory CLEMENT]

Hi Wolfram,

On 13/02/2014 21:36, Wolfram Sang wrote:
> So, this is a series I came up with trying to fix the issue found by Kevin.
> Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
> Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
> are further cleanup possibilities. And there is still more potential, I mainly
> wanted to give some inspiration and awareness that the driver could need some
> more love. Please test at least 1+2, comments to 3-5 very welcome.
> 
> Sorry for the delay, I got distracted by an NMI.
> 

Thanks for this series, indeed the code looks better.

I will test it tomorrow and let you know if it fixed the bug.
I will also take time to review the RFC patches.

Greogry


> Wolfram Sang (5):
>   i2c: mv64xxx: put offload check into offload prepare function
>   i2c: mv64xxx: refactor message start to ensure proper initialization
>   i2c: mv64xxx: refactor send_start
>   i2c: mv64xxx: directly call send_start when initializing transfer
>   i2c: mv64xxx: refactor initialization for new msgs
> 
>  drivers/i2c/busses/i2c-mv64xxx.c | 67 ++++++++++++++++------------------------
>  1 file changed, 27 insertions(+), 40 deletions(-)
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/5] mv64xxx updates

[Thomas Petazzoni]

Dear Wolfram Sang,

On Thu, 13 Feb 2014 21:36:28 +0100, Wolfram Sang wrote:
> So, this is a series I came up with trying to fix the issue found by Kevin.
> Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
> Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
> are further cleanup possibilities. And there is still more potential, I mainly
> wanted to give some inspiration and awareness that the driver could need some
> more love. Please test at least 1+2, comments to 3-5 very welcome.
> 
> Sorry for the delay, I got distracted by an NMI.

I'm adding Maxime Ripard in Cc, since he is the maintainer of the
Allwinner platform, which also uses this I2C driver. So I guess he
would like to be notified of such changes in order to test that the
driver still works for him.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 0/5] mv64xxx updates

[Thomas Petazzoni]

Dear Wolfram Sang,

On Thu, 13 Feb 2014 21:36:28 +0100, Wolfram Sang wrote:
> So, this is a series I came up with trying to fix the issue found by Kevin.
> Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
> Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
> are further cleanup possibilities. And there is still more potential, I mainly
> wanted to give some inspiration and awareness that the driver could need some
> more love. Please test at least 1+2, comments to 3-5 very welcome.
> 
> Sorry for the delay, I got distracted by an NMI.

I'm adding Maxime Ripard in Cc, since he is the maintainer of the
Allwinner platform, which also uses this I2C driver. So I guess he
would like to be notified of such changes in order to test that the
driver still works for him.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/5] i2c: mv64xxx: put offload check into offload prepare function

[Gregory CLEMENT]

On 13/02/2014 21:36, Wolfram Sang wrote:
> It makes code simpler to read and prepares a reorganization needed for a
> bugfix.
> 
> Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>

Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

As expected it didn't fix the issue I had, but it didn't introduce
any visible regression too.

> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index b8c5187..ba64978 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -204,6 +204,9 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
>  	unsigned long ctrl_reg;
>  	struct i2c_msg *msg = drv_data->msgs;
>  
> +	if (!drv_data->offload_enabled)
> +		return -EOPNOTSUPP;
> +
>  	drv_data->msg = msg;
>  	drv_data->byte_posn = 0;
>  	drv_data->bytes_left = msg->len;
> @@ -433,8 +436,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  
>  		drv_data->msgs++;
>  		drv_data->num_msgs--;
> -		if (!(drv_data->offload_enabled &&
> -				mv64xxx_i2c_offload_msg(drv_data))) {
> +		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
>  			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
>  			writel(drv_data->cntl_bits,
>  			drv_data->reg_base + drv_data->reg_offsets.control);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/5] i2c: mv64xxx: put offload check into offload prepare function

[Gregory CLEMENT]

On 13/02/2014 21:36, Wolfram Sang wrote:
> It makes code simpler to read and prepares a reorganization needed for a
> bugfix.
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

As expected it didn't fix the issue I had, but it didn't introduce
any visible regression too.

> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index b8c5187..ba64978 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -204,6 +204,9 @@ static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
>  	unsigned long ctrl_reg;
>  	struct i2c_msg *msg = drv_data->msgs;
>  
> +	if (!drv_data->offload_enabled)
> +		return -EOPNOTSUPP;
> +
>  	drv_data->msg = msg;
>  	drv_data->byte_posn = 0;
>  	drv_data->bytes_left = msg->len;
> @@ -433,8 +436,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  
>  		drv_data->msgs++;
>  		drv_data->num_msgs--;
> -		if (!(drv_data->offload_enabled &&
> -				mv64xxx_i2c_offload_msg(drv_data))) {
> +		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
>  			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
>  			writel(drv_data->cntl_bits,
>  			drv_data->reg_base + drv_data->reg_offsets.control);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 2/5] i2c: mv64xxx: refactor message start to ensure proper initialization

[Gregory CLEMENT]

On 13/02/2014 21:36, Wolfram Sang wrote:
> Because the offload mechanism can fall back to a standard transfer,
> having two seperate initialization states is unfortunate. Let's just
> have one state which does things consistently. This fixes a bug where
> some preparation was missing when the fallback happened. And it makes
> the code much easier to follow.
> 
> Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>


Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

With this one the bug we observed was fixed.


Thanks,

Gregory


> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index ba64978..d52d849 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -97,7 +97,6 @@ enum {
>  enum {
>  	MV64XXX_I2C_ACTION_INVALID,
>  	MV64XXX_I2C_ACTION_CONTINUE,
> -	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
>  	MV64XXX_I2C_ACTION_SEND_START,
>  	MV64XXX_I2C_ACTION_SEND_RESTART,
>  	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
> @@ -460,15 +459,14 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  			drv_data->reg_base + drv_data->reg_offsets.control);
>  		break;
>  
> -	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
> -		if (!mv64xxx_i2c_offload_msg(drv_data))
> -			break;
> -		else
> -			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> -		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
> -		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -			drv_data->reg_base + drv_data->reg_offsets.control);
> +		/* Can we offload this msg ? */
> +		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> +			/* No, switch to standard path */
> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> +				drv_data->reg_base + drv_data->reg_offsets.control);
> +		}
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
> @@ -627,15 +625,10 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
>  	unsigned long	flags;
>  
>  	spin_lock_irqsave(&drv_data->lock, flags);
> -	if (drv_data->offload_enabled) {
> -		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
> -		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
> -	} else {
> -		mv64xxx_i2c_prepare_for_io(drv_data, msg);
>  
> -		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> -		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
> -	}
> +	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
> +
>  	drv_data->send_stop = is_last;
>  	drv_data->block = 1;
>  	mv64xxx_i2c_do_action(drv_data);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/5] i2c: mv64xxx: refactor message start to ensure proper initialization

[Gregory CLEMENT]

On 13/02/2014 21:36, Wolfram Sang wrote:
> Because the offload mechanism can fall back to a standard transfer,
> having two seperate initialization states is unfortunate. Let's just
> have one state which does things consistently. This fixes a bug where
> some preparation was missing when the fallback happened. And it makes
> the code much easier to follow.
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>


Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

With this one the bug we observed was fixed.


Thanks,

Gregory


> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index ba64978..d52d849 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -97,7 +97,6 @@ enum {
>  enum {
>  	MV64XXX_I2C_ACTION_INVALID,
>  	MV64XXX_I2C_ACTION_CONTINUE,
> -	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
>  	MV64XXX_I2C_ACTION_SEND_START,
>  	MV64XXX_I2C_ACTION_SEND_RESTART,
>  	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
> @@ -460,15 +459,14 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  			drv_data->reg_base + drv_data->reg_offsets.control);
>  		break;
>  
> -	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
> -		if (!mv64xxx_i2c_offload_msg(drv_data))
> -			break;
> -		else
> -			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> -		/* FALLTHRU */
>  	case MV64XXX_I2C_ACTION_SEND_START:
> -		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -			drv_data->reg_base + drv_data->reg_offsets.control);
> +		/* Can we offload this msg ? */
> +		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> +			/* No, switch to standard path */
> +			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> +				drv_data->reg_base + drv_data->reg_offsets.control);
> +		}
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
> @@ -627,15 +625,10 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
>  	unsigned long	flags;
>  
>  	spin_lock_irqsave(&drv_data->lock, flags);
> -	if (drv_data->offload_enabled) {
> -		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
> -		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
> -	} else {
> -		mv64xxx_i2c_prepare_for_io(drv_data, msg);
>  
> -		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> -		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
> -	}
> +	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
> +	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
> +
>  	drv_data->send_stop = is_last;
>  	drv_data->block = 1;
>  	mv64xxx_i2c_do_action(drv_data);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3/5] i2c: mv64xxx: refactor send_start

[Gregory CLEMENT]

On 13/02/2014 21:36, Wolfram Sang wrote:
> For start and restart, we are doing the same thing. Let's consolidate
> that.
> 
> Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>

With my first tests it continue to work with this change,
but I want to have a closer look on it

Thanks,

Gregory


> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index d52d849..9c37b59 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -419,6 +419,17 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>  	}
>  }
>  
> +static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
> +{
> +	/* Can we offload this msg ? */
> +	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> +		/* No, switch to standard path */
> +		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> +			drv_data->reg_base + drv_data->reg_offsets.control);
> +	}
> +}
> +
>  static void
>  mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  {
> @@ -435,14 +446,11 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  
>  		drv_data->msgs++;
>  		drv_data->num_msgs--;
> -		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> -			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
> -			writel(drv_data->cntl_bits,
> -			drv_data->reg_base + drv_data->reg_offsets.control);
> +		// CHECKME: Does it work? Order of writel and prepare_for_io is
> +		// exchanged. Also, do we need to change cntl_bits in drv_data
> +		// with |= MV64XXX_I2C_REG_CONTROL_START?
> +		mv64xxx_i2c_send_start(drv_data);
>  
> -			/* Setup for the next message */
> -			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> -		}
>  		if (drv_data->errata_delay)
>  			udelay(5);
>  
> @@ -460,13 +468,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_START:
> -		/* Can we offload this msg ? */
> -		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> -			/* No, switch to standard path */
> -			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> -			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -				drv_data->reg_base + drv_data->reg_offsets.control);
> -		}
> +		mv64xxx_i2c_send_start(drv_data);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/5] i2c: mv64xxx: refactor send_start

[Gregory CLEMENT]

On 13/02/2014 21:36, Wolfram Sang wrote:
> For start and restart, we are doing the same thing. Let's consolidate
> that.
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

With my first tests it continue to work with this change,
but I want to have a closer look on it

Thanks,

Gregory


> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index d52d849..9c37b59 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -419,6 +419,17 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>  	}
>  }
>  
> +static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
> +{
> +	/* Can we offload this msg ? */
> +	if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> +		/* No, switch to standard path */
> +		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> +		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> +			drv_data->reg_base + drv_data->reg_offsets.control);
> +	}
> +}
> +
>  static void
>  mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  {
> @@ -435,14 +446,11 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  
>  		drv_data->msgs++;
>  		drv_data->num_msgs--;
> -		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> -			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
> -			writel(drv_data->cntl_bits,
> -			drv_data->reg_base + drv_data->reg_offsets.control);
> +		// CHECKME: Does it work? Order of writel and prepare_for_io is
> +		// exchanged. Also, do we need to change cntl_bits in drv_data
> +		// with |= MV64XXX_I2C_REG_CONTROL_START?
> +		mv64xxx_i2c_send_start(drv_data);
>  
> -			/* Setup for the next message */
> -			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> -		}
>  		if (drv_data->errata_delay)
>  			udelay(5);
>  
> @@ -460,13 +468,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_START:
> -		/* Can we offload this msg ? */
> -		if (mv64xxx_i2c_offload_msg(drv_data) < 0) {
> -			/* No, switch to standard path */
> -			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
> -			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -				drv_data->reg_base + drv_data->reg_offsets.control);
> -		}
> +		mv64xxx_i2c_send_start(drv_data);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 2/5] i2c: mv64xxx: refactor message start to ensure proper initialization

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Fri, Feb 14, 2014 at 12:39:02PM +0100, Gregory CLEMENT wrote:
> On 13/02/2014 21:36, Wolfram Sang wrote:
> > Because the offload mechanism can fall back to a standard transfer,
> > having two seperate initialization states is unfortunate. Let's just
> > have one state which does things consistently. This fixes a bug where
> > some preparation was missing when the fallback happened. And it makes
> > the code much easier to follow.
> > 
> > Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> 
> 
> Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> With this one the bug we observed was fixed.

I squashed patches 1+2 (easier handling for stable) and applied to
for-current, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/5] i2c: mv64xxx: refactor message start to ensure proper initialization

[Wolfram Sang]

On Fri, Feb 14, 2014 at 12:39:02PM +0100, Gregory CLEMENT wrote:
> On 13/02/2014 21:36, Wolfram Sang wrote:
> > Because the offload mechanism can fall back to a standard transfer,
> > having two seperate initialization states is unfortunate. Let's just
> > have one state which does things consistently. This fixes a bug where
> > some preparation was missing when the fallback happened. And it makes
> > the code much easier to follow.
> > 
> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> 
> 
> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> With this one the bug we observed was fixed.

I squashed patches 1+2 (easier handling for stable) and applied to
for-current, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140215/5266b037/attachment.sig>

Re: [PATCH 0/5] mv64xxx updates

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

Hi,

On Thu, Feb 13, 2014 at 09:51:00PM +0100, Thomas Petazzoni wrote:
> Dear Wolfram Sang,
> 
> On Thu, 13 Feb 2014 21:36:28 +0100, Wolfram Sang wrote:
> > So, this is a series I came up with trying to fix the issue found by Kevin.
> > Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
> > Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
> > are further cleanup possibilities. And there is still more potential, I mainly
> > wanted to give some inspiration and awareness that the driver could need some
> > more love. Please test at least 1+2, comments to 3-5 very welcome.
> > 
> > Sorry for the delay, I got distracted by an NMI.
> 
> I'm adding Maxime Ripard in Cc, since he is the maintainer of the
> Allwinner platform, which also uses this I2C driver. So I guess he
> would like to be notified of such changes in order to test that the
> driver still works for him.

I just gave a try to these patches, on my A31 board with the extra
patches sent previously.

Tested-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 0/5] mv64xxx updates

[Maxime Ripard]

Hi,

On Thu, Feb 13, 2014 at 09:51:00PM +0100, Thomas Petazzoni wrote:
> Dear Wolfram Sang,
> 
> On Thu, 13 Feb 2014 21:36:28 +0100, Wolfram Sang wrote:
> > So, this is a series I came up with trying to fix the issue found by Kevin.
> > Patches 1+2 are hopefully fixing the bug (in theory, I don't have the HW).
> > Patches 3-5 are RFC, and if patch 3 actually works (see the CHECKME), then 4+5
> > are further cleanup possibilities. And there is still more potential, I mainly
> > wanted to give some inspiration and awareness that the driver could need some
> > more love. Please test at least 1+2, comments to 3-5 very welcome.
> > 
> > Sorry for the delay, I got distracted by an NMI.
> 
> I'm adding Maxime Ripard in Cc, since he is the maintainer of the
> Allwinner platform, which also uses this I2C driver. So I guess he
> would like to be notified of such changes in order to test that the
> driver still works for him.

I just gave a try to these patches, on my A31 board with the extra
patches sent previously.

Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140218/3a3f8b4c/attachment.sig>

Re: [PATCH 0/5] mv64xxx updates

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 157 bytes --]


> I will test it tomorrow and let you know if it fixed the bug.
> I will also take time to review the RFC patches.

I applied the other three patches now.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 0/5] mv64xxx updates

[Wolfram Sang]

> I will test it tomorrow and let you know if it fixed the bug.
> I will also take time to review the RFC patches.

I applied the other three patches now.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140310/bdaf1ac8/attachment.sig>