i2c-gpio performance issue w/MPC8xx @ 50 MHz

i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Eran Duchan]

Hey list

I needed to bit bang I2C on an ancient MPC875 running at a blazing 50
MHz. I started by initializing the generic i2c-gpio bus but quickly
found it could not bit bang @ 100 kHz. The most I was able to get it
was ~50 kHz by setting udelay to 0 and patching around i2c-gpio.c to
make sure it doesn't modify this value.

I then implemented a bus driver of my own, based on i2c-gpio. The only
difference was that it simply implemented set/get sda/scl by directly
writing to hardware. Using this I was able to reach around 150 kHz.

1) Any comments on the above? Could I have done something differently?
2) I thought perhaps a thin adapter could be implemented that exported
a platform data structure holding callbacks: set sda/scl dir/val, get
sda/scl. By registering such a bus driver and supplying these
callbacks in the platform module, one could avoid having to
re-implement a bus driver like I did. Comments?

Thanks
Eran

Re: i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Jean Delvare]

Hi Eran,

On Tue, 19 Apr 2011 23:53:11 +0300, Eran Duchan wrote:
> Hey list
> 
> I needed to bit bang I2C on an ancient MPC875 running at a blazing 50
> MHz. I started by initializing the generic i2c-gpio bus but quickly
> found it could not bit bang @ 100 kHz. The most I was able to get it
> was ~50 kHz by setting udelay to 0 and patching around i2c-gpio.c to
> make sure it doesn't modify this value.
> 
> I then implemented a bus driver of my own, based on i2c-gpio. The only
> difference was that it simply implemented set/get sda/scl by directly
> writing to hardware. Using this I was able to reach around 150 kHz.
> 
> 1) Any comments on the above? Could I have done something differently?

Apparently the gpio layer is adding a delay somehow. You should ask
Grant Likely (gpio subsystem maintainer) and Haavard Skinnemoen (author
and maintainer of the i2c-gpio driver) about it. I Cc'd both. They
should know where in i2c-gpio or the gpio layer an extra delay may be
introduced.

> 2) I thought perhaps a thin adapter could be implemented that exported
> a platform data structure holding callbacks: set sda/scl dir/val, get
> sda/scl. By registering such a bus driver and supplying these
> callbacks in the platform module, one could avoid having to
> re-implement a bus driver like I did. Comments?

What you are proposing is turning i2c-algo-bit into a platform driver
instead of the basic library it is at the moment. When the code was
written, the platform "bus" didn't even exist so it wasn't an option.
While I don't expect all users of i2c-algo-bit to be converted to such
a platform driver, it may indeed be useful in some cases, so I have no
objection.

That being said, if you use GPIOs, it would certainly be better to find
out whether the extra delay you've been observing could be removed.
Properly requesting the GPIOs you need is definitely cleaner than
accessing the I/O port (or whatever) directly. Direct access can be
unreliable if other code is using GPIOs nearby.

-- 
Jean Delvare

Re: i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Håvard Skinnemoen]

Hi Eran,

On Sun, May 1, 2011 at 1:26 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Eran,
>
> On Tue, 19 Apr 2011 23:53:11 +0300, Eran Duchan wrote:
>> Hey list
>>
>> I needed to bit bang I2C on an ancient MPC875 running at a blazing 50
>> MHz. I started by initializing the generic i2c-gpio bus but quickly
>> found it could not bit bang @ 100 kHz. The most I was able to get it
>> was ~50 kHz by setting udelay to 0 and patching around i2c-gpio.c to
>> make sure it doesn't modify this value.

Hmm. i2c-gpio overrides the udelay specified by the platform? That
shouldn't happen. It does try to come up with some reasonable defaults
if the platform doesn't specify any though.

>> I then implemented a bus driver of my own, based on i2c-gpio. The only
>> difference was that it simply implemented set/get sda/scl by directly
>> writing to hardware. Using this I was able to reach around 150 kHz.
>>
>> 1) Any comments on the above? Could I have done something differently?
>
> Apparently the gpio layer is adding a delay somehow. You should ask
> Grant Likely (gpio subsystem maintainer) and Haavard Skinnemoen (author
> and maintainer of the i2c-gpio driver) about it. I Cc'd both. They
> should know where in i2c-gpio or the gpio layer an extra delay may be
> introduced.

I haven't investigated this in depth, but I'd say it's probably caused
by the extra indirection added by gpiolib, assuming the gpio framework
on your platform uses it. I guess it's one of the downsides of greatly
reducing the amount of platform-specific code.

>> 2) I thought perhaps a thin adapter could be implemented that exported
>> a platform data structure holding callbacks: set sda/scl dir/val, get
>> sda/scl. By registering such a bus driver and supplying these
>> callbacks in the platform module, one could avoid having to
>> re-implement a bus driver like I did. Comments?
>
> What you are proposing is turning i2c-algo-bit into a platform driver
> instead of the basic library it is at the moment. When the code was
> written, the platform "bus" didn't even exist so it wasn't an option.
> While I don't expect all users of i2c-algo-bit to be converted to such
> a platform driver, it may indeed be useful in some cases, so I have no
> objection.

You can probably create a minimal i2c-platform driver which just
passes the platform-specific callbacks to i2c-algo-bit. I think it
should be a new bus driver though...passing those callbacks to
i2c-gpio would basically eliminate the very small amount of
functionality this driver provides while making it more complex.

i2c-gpio currently has 225 lines of code. I think it should be
possible to write a i2c-platform driver in less than 100 lines.
Probably worthwhile for 3x performance, although probably not if the
GPIO hardware has concurrency issues.

> That being said, if you use GPIOs, it would certainly be better to find
> out whether the extra delay you've been observing could be removed.
> Properly requesting the GPIOs you need is definitely cleaner than
> accessing the I/O port (or whatever) directly. Direct access can be
> unreliable if other code is using GPIOs nearby.

If your hardware allows you to control individual pins without
affecting others, it's probably ok to bypass the GPIO layer (although
you should probably still request the pins to prevent others from
using them). If not, you should expect a lot of trouble.

Havard

Re: i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Eran Duchan]

Hey guys,

Thanks for taking a look.

> Hmm. i2c-gpio overrides the udelay specified by the platform? That
> shouldn't happen. It does try to come up with some reasonable defaults
> if the platform doesn't specify any though.

Yep, only this scheme explicitly disallows a udelay of 0. It's not too
problematic, I guess, seeing how close 1 is to 0 in terms of timing.

> I haven't investigated this in depth, but I'd say it's probably caused
> by the extra indirection added by gpiolib, assuming the gpio framework
> on your platform uses it. I guess it's one of the downsides of greatly
> reducing the amount of platform-specific code.

Indeed. Tracing the platform gpio code does not show it to be overly
complex at all; simply a matter of mapping the gpio # to a chip (which
is mapped to a port bank), mapping to the specific bit in the bank,
some locking, setting and unlocking. The only problem is that while
this is straightforward, it does introduce unwanted latency. I did not
get to profiling the code, though.

>> While I don't expect all users of i2c-algo-bit to be converted to such
>> a platform driver, it may indeed be useful in some cases, so I have no
>> objection.

Nor would I expect this to be the case. i2c-algo-bit obviously works
well on most platforms when paired to the gpio facility. It just isn't
tight enough for environments low on processing power.

> You can probably create a minimal i2c-platform driver which just
> passes the platform-specific callbacks to i2c-algo-bit. I think it
> should be a new bus driver though...

This is where I kind of lost you. I implemented a bus driver specific
to my platform (compiled in in drivers/i2c/busses) and then
instantiated it in my platform module. I thought about adding a
generic bus driver that would receive callbacks from the platform
module (for example, drivers/i2c/busses/i2c-gpio-direct, or something)
and simply pass these on to i2c-bit-algo. This would allow other
platforms to do the same without having to create a platform specific
bus driver each time like I did. I can submit something generic like
this, if you think this it conforms to the model.

Eran

Re: i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Håvard Skinnemoen]

On Mon, May 2, 2011 at 1:48 AM, Eran Duchan <pavius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hmm. i2c-gpio overrides the udelay specified by the platform? That
>> shouldn't happen. It does try to come up with some reasonable defaults
>> if the platform doesn't specify any though.
>
> Yep, only this scheme explicitly disallows a udelay of 0. It's not too
> problematic, I guess, seeing how close 1 is to 0 in terms of timing.

Hmm, yeah. Not sure how to allow 0 while still being backwards compatible.

>> I haven't investigated this in depth, but I'd say it's probably caused
>> by the extra indirection added by gpiolib, assuming the gpio framework
>> on your platform uses it. I guess it's one of the downsides of greatly
>> reducing the amount of platform-specific code.
>
> Indeed. Tracing the platform gpio code does not show it to be overly
> complex at all; simply a matter of mapping the gpio # to a chip (which
> is mapped to a port bank), mapping to the specific bit in the bank,
> some locking, setting and unlocking. The only problem is that while
> this is straightforward, it does introduce unwanted latency. I did not
> get to profiling the code, though.

Well, not really complex as such, but when you want to do all that
every 10 microseconds, it adds up really quickly.

>>> While I don't expect all users of i2c-algo-bit to be converted to such
>>> a platform driver, it may indeed be useful in some cases, so I have no
>>> objection.
>
> Nor would I expect this to be the case. i2c-algo-bit obviously works
> well on most platforms when paired to the gpio facility. It just isn't
> tight enough for environments low on processing power.
>
>> You can probably create a minimal i2c-platform driver which just
>> passes the platform-specific callbacks to i2c-algo-bit. I think it
>> should be a new bus driver though...
>
> This is where I kind of lost you. I implemented a bus driver specific
> to my platform (compiled in in drivers/i2c/busses) and then
> instantiated it in my platform module. I thought about adding a
> generic bus driver that would receive callbacks from the platform
> module (for example, drivers/i2c/busses/i2c-gpio-direct, or something)
> and simply pass these on to i2c-bit-algo. This would allow other
> platforms to do the same without having to create a platform specific
> bus driver each time like I did. I can submit something generic like
> this, if you think this it conforms to the model.

That sounds exactly like what I had in mind, except for the naming. If
the driver doesn't use the generic GPIO facilities at all, I don't
think it should have "gpio" in its name. What the driver does,
however, is taking platform-specific callbacks and passes them to
i2c-algo-bit. So I think "i2c-platform" would be a better name.

I don't really know whether it conforms to the "model" (or which model
we're talking about for that matter), but I can't see any problems
with it. Platforms for which i2c-gpio doesn't perform well enough can
get better performance at the cost of a little bit more
platform-specific code if they switch to i2c-platform.

Havard

Re: i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Jean Delvare]

Hi Håvard,

On Thu, 5 May 2011 11:54:44 -0700, Håvard Skinnemoen wrote:
> On Mon, May 2, 2011 at 1:48 AM, Eran Duchan <pavius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> Hmm. i2c-gpio overrides the udelay specified by the platform? That
> >> shouldn't happen. It does try to come up with some reasonable defaults
> >> if the platform doesn't specify any though.
> >
> > Yep, only this scheme explicitly disallows a udelay of 0. It's not too
> > problematic, I guess, seeing how close 1 is to 0 in terms of timing.
> 
> Hmm, yeah. Not sure how to allow 0 while still being backwards compatible.

You don't want to allow 0 anyway, as this is wrong. With 0 you can't
make _any_ timing guarantee, it might go faster than desired depending
on the CPU. You don't want I2C to break when the vendor implements a
faster CPU in their next product.

As said before, if the GPIO layer is too slow for such systems, then
either it has to be improved (if possible at all, maybe be enabling
some caching of the values which keep being recomputed for frequently
used GPIO pins?) or these platforms have to do without it.

> >> I haven't investigated this in depth, but I'd say it's probably caused
> >> by the extra indirection added by gpiolib, assuming the gpio framework
> >> on your platform uses it. I guess it's one of the downsides of greatly
> >> reducing the amount of platform-specific code.
> >
> > Indeed. Tracing the platform gpio code does not show it to be overly
> > complex at all; simply a matter of mapping the gpio # to a chip (which
> > is mapped to a port bank), mapping to the specific bit in the bank,
> > some locking, setting and unlocking. The only problem is that while
> > this is straightforward, it does introduce unwanted latency. I did not
> > get to profiling the code, though.
> 
> Well, not really complex as such, but when you want to do all that
> every 10 microseconds, it adds up really quickly.
> 
> >>> While I don't expect all users of i2c-algo-bit to be converted to such
> >>> a platform driver, it may indeed be useful in some cases, so I have no
> >>> objection.
> >
> > Nor would I expect this to be the case. i2c-algo-bit obviously works
> > well on most platforms when paired to the gpio facility. It just isn't
> > tight enough for environments low on processing power.
> >
> >> You can probably create a minimal i2c-platform driver which just
> >> passes the platform-specific callbacks to i2c-algo-bit. I think it
> >> should be a new bus driver though...
> >
> > This is where I kind of lost you. I implemented a bus driver specific
> > to my platform (compiled in in drivers/i2c/busses) and then
> > instantiated it in my platform module. I thought about adding a
> > generic bus driver that would receive callbacks from the platform
> > module (for example, drivers/i2c/busses/i2c-gpio-direct, or something)
> > and simply pass these on to i2c-bit-algo. This would allow other
> > platforms to do the same without having to create a platform specific
> > bus driver each time like I did. I can submit something generic like
> > this, if you think this it conforms to the model.
> 
> That sounds exactly like what I had in mind, except for the naming. If
> the driver doesn't use the generic GPIO facilities at all, I don't
> think it should have "gpio" in its name. What the driver does,

I agree.

> however, is taking platform-specific callbacks and passes them to
> i2c-algo-bit. So I think "i2c-platform" would be a better name.

Err, no, i2c-platform is no better than i2c-gpio-direct, I'm afraid.
There are many "platform" i2c bus drivers already, so that would be
confusing. I'd vote for "i2c-bitbang", or "i2c-bit-platform" to match
"i2c-pca-platform" we already have for i2c-algo-pca.

> I don't really know whether it conforms to the "model" (or which model
> we're talking about for that matter), but I can't see any problems
> with it. Platforms for which i2c-gpio doesn't perform well enough can
> get better performance at the cost of a little bit more
> platform-specific code if they switch to i2c-platform.

Agreed. As long as care is taken that other drivers won't poke at the
same registers, obviously.

-- 
Jean Delvare

Re: i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Ben Dooks]

On Sun, May 01, 2011 at 03:04:21PM -0700, Håvard Skinnemoen wrote:
> Hi Eran,
> 
> Hmm. i2c-gpio overrides the udelay specified by the platform? That
> shouldn't happen. It does try to come up with some reasonable defaults
> if the platform doesn't specify any though.

[snip]

> You can probably create a minimal i2c-platform driver which just
> passes the platform-specific callbacks to i2c-algo-bit. I think it
> should be a new bus driver though...passing those callbacks to
> i2c-gpio would basically eliminate the very small amount of
> functionality this driver provides while making it more complex.
> 
> i2c-gpio currently has 225 lines of code. I think it should be
> possible to write a i2c-platform driver in less than 100 lines.
> Probably worthwhile for 3x performance, although probably not if the
> GPIO hardware has concurrency issues.

I suppose this would work, but is it worth the additional code for what
may not be much of a win?
 
> > That being said, if you use GPIOs, it would certainly be better to find
> > out whether the extra delay you've been observing could be removed.
> > Properly requesting the GPIOs you need is definitely cleaner than
> > accessing the I/O port (or whatever) directly. Direct access can be
> > unreliable if other code is using GPIOs nearby.
> 
> If your hardware allows you to control individual pins without
> affecting others, it's probably ok to bypass the GPIO layer (although
> you should probably still request the pins to prevent others from
> using them). If not, you should expect a lot of trouble.

yeuck.

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

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: i2c-gpio performance issue w/MPC8xx @ 50 MHz

[Håvard Skinnemoen]

On Wed, May 4, 2011 at 2:50 PM, Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> On Sun, May 01, 2011 at 03:04:21PM -0700, Håvard Skinnemoen wrote:
>> i2c-gpio currently has 225 lines of code. I think it should be
>> possible to write a i2c-platform driver in less than 100 lines.
>> Probably worthwhile for 3x performance, although probably not if the
>> GPIO hardware has concurrency issues.
>
> I suppose this would work, but is it worth the additional code for what
> may not be much of a win?

3x performance is not much of a win?

Havard