Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT

[Wolfgang Grandegger <wg@grandegger.com>]

Jon Smirl wrote:
> On 7/31/08, Trent Piepho <xyzzy@speakeasy.org> wrote:
>> On Thu, 31 Jul 2008, Jon Smirl wrote:
>>  > As for the source clock, how about creating a new global like
>>  > ppc_proc_freq called ppc_ipb_freq. The platform code can then set the
>>  > right clock value into the variable. For mpc8xxxx get it from uboot.
>>  > mpc5200 can easily compute it from ppc_proc_freq and checking how the
>>  > ipb divider is set. That will move the clock problem out of the i2c
>>  > driver.
>>
>>
>> There is a huge variation in where the I2C source clock comes from.
>>  Sometimes it's the system bus, sometimes ethernet, sometimes SEC, etc.  If
>>  I look at u-boot (which might not be entirely correct or complete), I see:
>>
>>  83xx:  5 different clock sources
>>  85xx:  3 different clock sources
>>  86xx:  2 different clock sources
>>
>>  But there's more.  Sometimes the two I2C controllers don't use the same
>>  clock!  So even if you add 10 globals with different clocks, and then add
>>  code to the mpc i2c driver so if can figure out which one to use given the
>>  platform, it's still not enough because you need to know which controller
>>  the device node is for.
>>
>>  IMHO, what Timur suggested of having u-boot put the source clock into the
>>  i2c node makes the most sense.  U-boot has to figure this out, so why
>>  duplicate the work?
>>
>>  Here's my idea:
>>
>>         i2c@0 {
>>                 compatible = "fsl-i2c";
>>                 bus-frequency = <100000>;
>>
>>                 /* Either */
>>                 source-clock-frequency = <0>;
>>                 /* OR */
>>                 source-clock = <&ccb>;
>>         };
> 
> Can't we hide a lot of this on platforms where the source clock is not
> messed up? For example the mpc5200 doesn't need any of this, the
> needed frequency is already available in mpc52xx_find_ipb_freq().
> mpc5200 doesn't need any uboot change.
> 
> Next would be normal mpc8xxx platforms where i2c is driven by a single
> clock, add a uboot filled in parameter in the root node (or I think it
> can be computed off of the ones uboot is already filling in). make a
> mpc8xxx_find_i2c_freq() function. May not need to change device tree
> and uboot.
> 
> Finally use this for those days when the tea leaves were especially
> bad. Both a device tree and uboot change.
> 
>> Except the i2c clock isn't always a based on an integer divider of the CCB
>>  frequency.  What's more, it's not always the same for both i2c controllers.
>>  Suppose i2c #1 uses CCB times 2/3 and i2c #2 uses CCB/2, how would
>>  fsl_get_i2c_freq() figure that out from bus-frequency and
>>  i2c-clock-divider?
> 
> If you get the CCB frequency from uboot and know the chip model, can't
> you compute these in the platform code? Then make a
> mpc8xxx_find_i2c_freq(cell_index).

We can, of course, but do we want to? #ifdef's are not acceptable for 
Linux which means scanning the model property to get the divider from 
some table. And when a new MPC model shows up, we need to update the 
table. This can all be saved and avoided by adding a I2C clock source 
divider or frequency property to the FDT. The FDT is to describe the 
hardware and the fixed divider value is a property of it.

I'm in favor of a I2C node specific "divider" property because it does 
not rely on a boot-loader filling in the real value. It's fixed for a 
certain MPC model. And the I2C source clock frequency is then just:

   fsl_get_sys_freq() / divider.

I'm quite sure that work for all MPCs, but at least for the one covered 
by the i2c-mpc driver.

Furthermore, mpc52xx_find_ipb_freq() does the same as 
fsl_get_sys_freq(). It looks up the value for the property 
"bus-frequency" of the soc. We don't need a mpc8xxx_find_i2c_freq() but 
a common fsl_get_i2c_freq() for all MPCs.

Wolfgang.