i2c-mpc clocking scheme

i2c-mpc clocking scheme

[Andre Schwarz]

All,

is anybody working on some improvements regarding configurable I2C
frequency inside the i2c-mpc driver ?

If not - would anybody be intersted in getting this done, i.e.
configurable via device tree ?



regards,
Andre

MATRIX VISION GmbH, Talstraße 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschäftsführer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: i2c-mpc clocking scheme

[Timur Tabi]

On Thu, Nov 27, 2008 at 9:00 AM, Andre Schwarz
<andre.schwarz@matrix-vision.de> wrote:
> All,
>
> is anybody working on some improvements regarding configurable I2C
> frequency inside the i2c-mpc driver ?
>
> If not - would anybody be intersted in getting this done, i.e.
> configurable via device tree ?

Maybe I'm missing something, but U-Boot configures the I2C bus speed.
It does this because the algorithm is specific to the SOC itself.  For
example, the 8544 is different from the 8548.  It would be a mess to
duplicate this code in the kernel.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: i2c-mpc clocking scheme

[Andre Schwarz]

Timur Tabi schrieb:
> On Thu, Nov 27, 2008 at 9:00 AM, Andre Schwarz
> <andre.schwarz@matrix-vision.de> wrote:
>   
>> All,
>>
>> is anybody working on some improvements regarding configurable I2C
>> frequency inside the i2c-mpc driver ?
>>
>> If not - would anybody be intersted in getting this done, i.e.
>> configurable via device tree ?
>>     
>
> Maybe I'm missing something, but U-Boot configures the I2C bus speed.
> It does this because the algorithm is specific to the SOC itself.  For
> example, the 8544 is different from the 8548.  It would be a mess to
> duplicate this code in the kernel.
>
>   
You're right regarding U-Boot, but the i2c-mpc driver overwrites the
"frequency divider register" on all chips.

I'm not happy with a fixed 0x3f @ MPC5200 which results in 65kHz ... :-(

Have a look at line 163 in drivers/i2c/busses/i2c-mpc.c



MATRIX VISION GmbH, Talstraße 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschäftsführer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: i2c-mpc clocking scheme

[Luotao Fu]

Timur Tabi wrote:
> On Thu, Nov 27, 2008 at 9:00 AM, Andre Schwarz
> <andre.schwarz@matrix-vision.de> wrote:
>> All,
>>
>> is anybody working on some improvements regarding configurable I2C
>> frequency inside the i2c-mpc driver ?
>>
>> If not - would anybody be intersted in getting this done, i.e.
>> configurable via device tree ?
> 
> Maybe I'm missing something, but U-Boot configures the I2C bus speed.

i2c-mpc sets the bus clock in kernel with mpc_i2c_setclock. The bus
speed in kernel is indepedent from u-boot.

> It does this because the algorithm is specific to the SOC itself.  For
> example, the 8544 is different from the 8548.  It would be a mess to
> duplicate this code in the kernel.
> 

I did try to write some mechanismen to make the i2c bus speed
configurable for  mpc5200. I found it quite hard to find some algorithm
to calculate the right values in the Timing table of the I2C bus
controller. Anyhow I gave it up after starring at the datasheet for some
time. ;-) I can't speak for other platforms, which are supported by
i2c-mpc. Anyway, I do think it might be interesting to have configurable
i2c bus speed.

cheers
Luotao Fu
-- 
   Dipl.-Ing. Luotao Fu | Phone: +49-5121-206917-5004
Pengutronix - Linux Solutions for Science and Industry
Entwicklungszentrum Nord     http://www.pengutronix.de

Re: i2c-mpc clocking scheme

[Trent Piepho]

On Thu, 27 Nov 2008, Andre Schwarz wrote:
> Timur Tabi schrieb:
>> On Thu, Nov 27, 2008 at 9:00 AM, Andre Schwarz
>> <andre.schwarz@matrix-vision.de> wrote:
>>
>>> All,
>>>
>>> is anybody working on some improvements regarding configurable I2C
>>> frequency inside the i2c-mpc driver ?
>>>
>>> If not - would anybody be intersted in getting this done, i.e.
>>> configurable via device tree ?
>>>
>>
>> Maybe I'm missing something, but U-Boot configures the I2C bus speed.
>> It does this because the algorithm is specific to the SOC itself.  For
>> example, the 8544 is different from the 8548.  It would be a mess to
>> duplicate this code in the kernel.
>>
>>
> You're right regarding U-Boot, but the i2c-mpc driver overwrites the
> "frequency divider register" on all chips.
>
> I'm not happy with a fixed 0x3f @ MPC5200 which results in 65kHz ... :-(
>
> Have a look at line 163 in drivers/i2c/busses/i2c-mpc.c

Seems like it should keep the clock registers at what u-boot set them too.

Re: i2c-mpc clocking scheme

[Timur Tabi]

Trent Piepho wrote:

> Seems like it should keep the clock registers at what u-boot set them too.

Or we could have U-Boot put the i2c clock frequency into the I2C node, and let
the driver program the hardware again.  That would keep the ugliness in U-Boot.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: i2c-mpc clocking scheme

[André Schwarz]

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

Timur Tabi wrote:
> Trent Piepho wrote:
>
>   
>> Seems like it should keep the clock registers at what u-boot set them too.
>>     
>
> Or we could have U-Boot put the i2c clock frequency into the I2C node, and let
> the driver program the hardware again.  That would keep the ugliness in U-Boot.
>
>   
Wouldn't it be easier to omit frequency re-programming at all ?
Maybe configurable for non U-Boot users ...


MATRIX VISION GmbH, Talstraße 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschäftsführer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[-- Attachment #2: Type: text/html, Size: 1015 bytes --]

Re: i2c-mpc clocking scheme

[Timur Tabi]

André Schwarz wrote:

> Wouldn't it be easier to omit frequency re-programming at all ?
> Maybe configurable for non U-Boot users ...

Well, the real problem is that Linux is ignoring what the boot loader has done.
 This is bad, regardless as to which boot loader you're using.

The question is, do we give Linux the capability to program the I2C bus speed,
or is this something that the boot loader should do?  Unfortunately, the current
Linux code means that this issue is not properly architected.

You mentioned people who don't use U-Boot.  Do they use some other boot loader?
 Or no boot loader at all?  Do other boot loaders program the I2C bus speed?
The reason I ask is that I want to know whether it's okay for Linux to ignore
the FDR and DFSRR registers.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: i2c-mpc clocking scheme

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1256 bytes --]

On Mon, 1 Dec 2008, André Schwarz wrote:
> Timur Tabi wrote:
>>  Trent Piepho wrote:
>>
>> 
>> >  Seems like it should keep the clock registers at what u-boot set them 
>> >  too.
>> > 
>>
>>  Or we could have U-Boot put the i2c clock frequency into the I2C node, and
>>  let
>>  the driver program the hardware again.  That would keep the ugliness in
>>  U-Boot.
>>
>> 
> Wouldn't it be easier to omit frequency re-programming at all ?
> Maybe configurable for non U-Boot users ...

That's what I'm thinking too.  Calculating the settings for a given bus
frequency, even with the system specific source clock provided by u-boot,
involves either a complex algorithm or a big ugly black box table that's
even larger than the algorithm.  I also think it's different for 8xxx and
52xx.

On the other hand just keeping the clock the same doesn't require any code
at all.

U-boot could pass in "bus-frequency" to let software know the speed of the
I2C bus from Linux.  Seems like a standard property for bus nodes.

There could be a "current-speed" property that tells linux to keep the
registers the same, in case we have to worry about u-boot not programming
the i2c clock.  I think the serial drivers have something like this.

Re: i2c-mpc clocking scheme

[Scott Wood]

Trent Piepho wrote:
> U-boot could pass in "bus-frequency" to let software know the speed of the
> I2C bus from Linux.  Seems like a standard property for bus nodes.

clock-frequency is standard, though it should probably be the input 
frequency rather than the bus frequency, in case the OS really does want 
to change it (maybe making the bus run faster when accessing faster 
devices).

> There could be a "current-speed" property that tells linux to keep the
> registers the same,

That would be a bit different from the way it's used in serial nodes, 
where current-speed is simply a description of the baud rate that 
corresponds to the current divider setting.  I'm not sure that it makes 
as much sense for i2c, as you don't have the shared state on the other 
end that depends on maintaining the exact same speed.

When does the guest really care what the specific i2c bus frequency is, 
if it's not going to change it?

-Scott

Re: i2c-mpc clocking scheme

[Trent Piepho]

On Mon, 1 Dec 2008, Scott Wood wrote:
> Trent Piepho wrote:
>>  U-boot could pass in "bus-frequency" to let software know the speed of the
>>  I2C bus from Linux.  Seems like a standard property for bus nodes.
>
> clock-frequency is standard, though it should probably be the input frequency 
> rather than the bus frequency, in case the OS really does want to change it 
> (maybe making the bus run faster when accessing faster devices).

For a bus device like an i2c controller, you really have two clocks.  The
input clock the controller runs from and the speed it runs the bus at.  One
could say that one clock is for the device node and the other clock is for
the device's sub-nodes.

Linux doesn't have an API for setting I2C bus speed.  If it did, then
adding support for it to i2c-mpc if there was demand would really be
another patch.  Right now the problem is that Linux programs the I2C
controller stupidly and undoes u-boot's (better) settings.

>>  There could be a "current-speed" property that tells linux to keep the
>>  registers the same,
>
> That would be a bit different from the way it's used in serial nodes, where 
> current-speed is simply a description of the baud rate that corresponds to 
> the current divider setting.  I'm not sure that it makes as much sense for 
> i2c, as you don't have the shared state on the other end that depends on 
> maintaining the exact same speed.

The Linux code could use current-speed to know if it should program the
registers.  I.e., if current-speed is present and non-zero, then leave the
frequency registers alone.  Otherwise u-boot or whatever might not have
programmed the I2C controller and the driver can do what it's doing now.

> When does the guest really care what the specific i2c bus frequency is, if 
> it's not going to change it?

I don't know of a real reason.  Maybe an I2C device where the clock speed
makes a difference?  Maximum polling rate or something?  Is there reason
the CPU clock and the CCB frequency need to be in the device tree?

Re: i2c-mpc clocking scheme

[Timur Tabi]

Trent Piepho wrote:

> For a bus device like an i2c controller, you really have two clocks.  The
> input clock the controller runs from and the speed it runs the bus at.  One
> could say that one clock is for the device node and the other clock is for
> the device's sub-nodes.

We could add a property to each I2C device nodes that lists the maximum speed
that this supports.  Then the I2C driver could find the smallest of these
speeds, and program the I2C controller for that speed.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: i2c-mpc clocking scheme

[Scott Wood]

Trent Piepho wrote:
> The Linux code could use current-speed to know if it should program the
> registers.  I.e., if current-speed is present and non-zero, then leave the
> frequency registers alone.  Otherwise u-boot or whatever might not have
> programmed the I2C controller and the driver can do what it's doing now.

I suppose.  I was thinking that Linux could just check to see whether 
the current divider value appears to be valid, but it seems that all 
values including zero can be valid. :-(

>> When does the guest really care what the specific i2c bus frequency is, if 
>> it's not going to change it?
> 
> I don't know of a real reason.  Maybe an I2C device where the clock speed
> makes a difference?  Maximum polling rate or something?  Is there reason
> the CPU clock and the CCB frequency need to be in the device tree?

I'm fine with including it for informational purposes, it just doesn't 
seem quite as necessary.

-Scott