About a suspicious msleep during doxfer in s3c2410 i2c bus driver

About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Dongsoo, Nathaniel Kim]

Hello,

Since I was working on S3C64XX and C1XX, I found something weird on
I2C bus device.
The thing is that when I try to write multiple registers through I2C
bus, it takes much more time for multiple registers to be written than
any other processor's i2c bus device.
I tracked down for which point does it take and finally found out that
following msleep was making a tremendous delay when I try to write
thousands of registers through I2C bus. Please look at this.

481 static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
482                               struct i2c_msg *msgs, int num)
483 {

<snip>

521         /* ensure the stop has been through the bus */
522
523         msleep(1);


With several different target boards using s3c64xx and s5pc1xx, it
went good without that msleep(). And of course it is working great
with multiple i2c device attached as well.
So, I want to ask Ben and anyone else using I2C device about removing
that msleep might be ok or not. In my case, it went ok. Please let me
know your opinion.
Cheers,

Nate

-- 
=
DongSoo, Nathaniel Kim
Engineer
Mobile S/W Platform Lab.
Digital Media & Communications R&D Centre
Samsung Electronics CO., LTD.
e-mail : dongsoo.kim-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
          dongsoo45.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Mark Brown]

On Wed, Jul 22, 2009 at 05:12:17PM +0900, Dongsoo, Nathaniel Kim wrote:

> Since I was working on S3C64XX and C1XX, I found something weird on
> I2C bus device.
> The thing is that when I try to write multiple registers through I2C
> bus, it takes much more time for multiple registers to be written than
> any other processor's i2c bus device.

Yes, this is extremely painful for a lot of devices - audio CODECs tend
to be noticably affected, especially at resume.  Depending on your
device and what you're doing you *may* be able to speed things up by
accessing many registers in a single I/O in the chip driver but
obviously that's not ideal.

> With several different target boards using s3c64xx and s5pc1xx, it
> went good without that msleep(). And of course it is working great
> with multiple i2c device attached as well.
> So, I want to ask Ben and anyone else using I2C device about removing
> that msleep might be ok or not. In my case, it went ok. Please let me
> know your opinion.

My *recollection* is that this is mostly there for multi-master
configurations.

I'm wondering if even if we can't remove the delay completely yet we
could add platform data to allow it to be configured on a per-board
basis?

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Dongsoo Kim]

2009. 07. 22, 오후 9:43, Mark Brown 작성:

> On Wed, Jul 22, 2009 at 05:12:17PM +0900, Dongsoo, Nathaniel Kim  
> wrote:
>
>> Since I was working on S3C64XX and C1XX, I found something weird on
>> I2C bus device.
>> The thing is that when I try to write multiple registers through I2C
>> bus, it takes much more time for multiple registers to be written  
>> than
>> any other processor's i2c bus device.
>
> Yes, this is extremely painful for a lot of devices - audio CODECs  
> tend
> to be noticably affected, especially at resume.  Depending on your
> device and what you're doing you *may* be able to speed things up by
> accessing many registers in a single I/O in the chip driver but
> obviously that's not ideal.
>
>> With several different target boards using s3c64xx and s5pc1xx, it
>> went good without that msleep(). And of course it is working great
>> with multiple i2c device attached as well.
>> So, I want to ask Ben and anyone else using I2C device about removing
>> that msleep might be ok or not. In my case, it went ok. Please let me
>> know your opinion.
>
> My *recollection* is that this is mostly there for multi-master
> configurations.
>
> I'm wondering if even if we can't remove the delay completely yet we
> could add platform data to allow it to be configured on a per-board
> basis?

I  wish I could answer clearly about this but not having much  
experience over various I2C bus devices so I can't say with a strict  
"yes". But in my experience, I've never seen any i2c bus driver  
serving platform data to be configured with delay support. Tho if it  
is really necessary for some specified class of i2c devices, we should  
make one.
I'm wondering why anybody has been issued this topic yet. the driver  
for s3c2410 i2c bus has been the way it is for ages I guess. It  
obviously seems to be taking long time to write down registers through  
the s3c2410 i2c bus driver than any other processor's i2c driver.  
maybe not so many people using enormous i2c register programming on  
Samsung processors could be the reason I suppose. anyway, let's figure  
it out the best way we can.
Cheers,

Nate




=
Dongsoo, Nathaniel Kim
Engineer
Mobile S/W Platform Lab.
Digital Media & Communications R&D Centre
Samsung Electronics CO., LTD.
e-mail : dongsoo.kim-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
            dongsoo45.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Mark Brown]

On Thu, Jul 23, 2009 at 12:58:52AM +0900, Dongsoo Kim wrote:
> 2009. 07. 22, 오후 9:43, Mark Brown 작성:

> >My *recollection* is that this is mostly there for multi-master
> >configurations.

> I  wish I could answer clearly about this but not having much
> experience over various I2C bus devices so I can't say with a strict
> "yes". But in my experience, I've never seen any i2c bus driver
> serving platform data to be configured with delay support. Tho if it
> is really necessary for some specified class of i2c devices, we
> should make one.

It wasn't for the client devices, it was for issues with synchronisation
with the controller hardware.  I'm just going on the basis of
recollections of previous conversations with Ben here; I'm not sure to
what extent this might be an issue with the way the hardware works
requiring the driver to jump through hoops.

> I'm wondering why anybody has been issued this topic yet. the driver
> for s3c2410 i2c bus has been the way it is for ages I guess. It
> obviously seems to be taking long time to write down registers
> through the s3c2410 i2c bus driver than any other processor's i2c
> driver. maybe not so many people using enormous i2c register
> programming on Samsung processors could be the reason I suppose.
> anyway, let's figure it out the best way we can.
> Cheers,

I expect most people would, like you, much rather just remove the sleep
from the driver since in practice that seems to work well.  For various
reasons the S3C community isn't all that active upstream which doesn't
help with issues like this.

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Ben Dooks]

On Wed, Jul 22, 2009 at 05:06:40PM +0100, Mark Brown wrote:
> On Thu, Jul 23, 2009 at 12:58:52AM +0900, Dongsoo Kim wrote:
> > 2009. 07. 22, ?????? 9:43, Mark Brown ??????:
> 
> > >My *recollection* is that this is mostly there for multi-master
> > >configurations.
> 
> > I  wish I could answer clearly about this but not having much
> > experience over various I2C bus devices so I can't say with a strict
> > "yes". But in my experience, I've never seen any i2c bus driver
> > serving platform data to be configured with delay support. Tho if it
> > is really necessary for some specified class of i2c devices, we
> > should make one.
> 
> It wasn't for the client devices, it was for issues with synchronisation
> with the controller hardware.  I'm just going on the basis of
> recollections of previous conversations with Ben here; I'm not sure to
> what extent this might be an issue with the way the hardware works
> requiring the driver to jump through hoops.

I think this has been in here for a while, and may well not be
necessary any more. If anyone else has tested this without the
msleep() here, then I'd be interested to know.

I will try and sort out testing a few boards as soon as possible to
see what is going on.
 
> > I'm wondering why anybody has been issued this topic yet. the driver
> > for s3c2410 i2c bus has been the way it is for ages I guess. It
> > obviously seems to be taking long time to write down registers
> > through the s3c2410 i2c bus driver than any other processor's i2c
> > driver. maybe not so many people using enormous i2c register
> > programming on Samsung processors could be the reason I suppose.
> > anyway, let's figure it out the best way we can.
> > Cheers,
> 
> I expect most people would, like you, much rather just remove the sleep
> from the driver since in practice that seems to work well.  For various
> reasons the S3C community isn't all that active upstream which doesn't
> help with issues like this.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Dongsoo Kim]

Hi Ben,

2009. 07. 31, 오전 7:18, Ben Dooks 작성:

> On Wed, Jul 22, 2009 at 05:06:40PM +0100, Mark Brown wrote:
>> On Thu, Jul 23, 2009 at 12:58:52AM +0900, Dongsoo Kim wrote:
>>> 2009. 07. 22, ?????? 9:43, Mark Brown ??????:
>>
>>>> My *recollection* is that this is mostly there for multi-master
>>>> configurations.
>>
>>> I  wish I could answer clearly about this but not having much
>>> experience over various I2C bus devices so I can't say with a strict
>>> "yes". But in my experience, I've never seen any i2c bus driver
>>> serving platform data to be configured with delay support. Tho if it
>>> is really necessary for some specified class of i2c devices, we
>>> should make one.
>>
>> It wasn't for the client devices, it was for issues with  
>> synchronisation
>> with the controller hardware.  I'm just going on the basis of
>> recollections of previous conversations with Ben here; I'm not sure  
>> to
>> what extent this might be an issue with the way the hardware works
>> requiring the driver to jump through hoops.
>
> I think this has been in here for a while, and may well not be
> necessary any more. If anyone else has tested this without the
> msleep() here, then I'd be interested to know.
>
> I will try and sort out testing a few boards as soon as possible to
> see what is going on.
>

I've been tested without msleep on s3c64xx and also s5pc1xx. And it  
went well.
What I'm concerned is s3c24xx because I had no chance to test on it.
Cheers,

Nate

>>> I'm wondering why anybody has been issued this topic yet. the driver
>>> for s3c2410 i2c bus has been the way it is for ages I guess. It
>>> obviously seems to be taking long time to write down registers
>>> through the s3c2410 i2c bus driver than any other processor's i2c
>>> driver. maybe not so many people using enormous i2c register
>>> programming on Samsung processors could be the reason I suppose.
>>> anyway, let's figure it out the best way we can.
>>> Cheers,
>>
>> I expect most people would, like you, much rather just remove the  
>> sleep
>> from the driver since in practice that seems to work well.  For  
>> various
>> reasons the S3C community isn't all that active upstream which  
>> doesn't
>> help with issues like this.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> i2c" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> -- 
> Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
>
>  'a smiley only costs 4 bytes'

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Peter Korsgaard]

>>>>> "Ben" == Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> writes:

Hi,

 >> It wasn't for the client devices, it was for issues with
 >> synchronisation with the controller hardware.  I'm just going on
 >> the basis of recollections of previous conversations with Ben
 >> here; I'm not sure to what extent this might be an issue with the
 >> way the hardware works requiring the driver to jump through hoops.

 Ben> I think this has been in here for a while, and may well not be
 Ben> necessary any more. If anyone else has tested this without the
 Ben> msleep() here, then I'd be interested to know.

I'll test on my 6410 board over the weekend. Notice that I've been
hacking on the uboot drivers/i2c/s3c24x0_i2c.c file to add s3c64xx
support, and I there had to add a udelay before setting the start
condition to get it to work stable.

-- 
Bye, Peter Korsgaard

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Ben Dooks]

On Fri, Jul 31, 2009 at 10:05:00AM +0200, Peter Korsgaard wrote:
> >>>>> "Ben" == Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> writes:
> 
> Hi,
> 
>  >> It wasn't for the client devices, it was for issues with
>  >> synchronisation with the controller hardware.  I'm just going on
>  >> the basis of recollections of previous conversations with Ben
>  >> here; I'm not sure to what extent this might be an issue with the
>  >> way the hardware works requiring the driver to jump through hoops.
> 
>  Ben> I think this has been in here for a while, and may well not be
>  Ben> necessary any more. If anyone else has tested this without the
>  Ben> msleep() here, then I'd be interested to know.
> 
> I'll test on my 6410 board over the weekend. Notice that I've been
> hacking on the uboot drivers/i2c/s3c24x0_i2c.c file to add s3c64xx
> support, and I there had to add a udelay before setting the start
> condition to get it to work stable.

does it check for bus busy before starting?

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: About a suspicious msleep during doxfer in s3c2410 i2c bus driver

[Mark Brown]

On Thu, Jul 30, 2009 at 11:18:41PM +0100, Ben Dooks wrote:

> I think this has been in here for a while, and may well not be
> necessary any more. If anyone else has tested this without the
> msleep() here, then I'd be interested to know.

I didn't notice any issues when I tested on s3c64xx systems but this
wasn't a particularly scientific test.