Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote:
On 7/31/08, Trent Piepho [EMAIL PROTECTED] 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 mpc8 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:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
bus-frequency = 10;
/* 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.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Trent Piepho wrote: On Thu, 31 Jul 2008, Jon Smirl wrote: [...snip...] I don't see why we want to go through the trouble of having uboot tell us things about a chip that are fixed in stone. Obviously something like the frequency of the external crystal needs to be passed up, but why pass up stuff that can be computed (or recovered by reading a register)? One could also say that if U-boot has to have the code and already calculated the value, why duplicate the code and the calculation in Linux? Right, if the bus-frequency property for the I2C device is not defined, the Linux I2C bus driver should just overtake the pre-defined values. That's what I (we?) wanted to implement anyhow. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 6:32 PM, Trent Piepho [EMAIL PROTECTED] wrote: The real problem, which kept me from making a patch to do this months ago, is that the source clock that the I2C freq divider applies to is different for just about every MPC8xxx platform. Not just a different value, but a totally different internal clock. Sometimes is CCB, sometimes CCB/2, sometimes tsec2's clock, etc. On which SOC is it the tesc2 clock? Sometimes the two i2c controllers don't even have the same clock. On which platform is that the case? I thought I had all 8[356]xx boards covered. Did I miss some? -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl [EMAIL PROTECTED] wrote: I've having trouble with whether these clocks are a system resource or something that belongs to i2c. If they are a system resource then we should make nodes in the root and use a phandle in the i2c node to link to them. I can't speak for 52xx, but for 8[356]xx, I would say the clocks belong to the I2C devices. The screwball determination of whether to divide by 1, 2, or 3 only applies to the I2C device only. The divider itself is not used to drive a clock for any other device. If you disable I2C support, then you don't need to care about the divider (or its output clock) at all. That's why I think have the I2C clock in the parent node is wrong, because it would only be used if you had I2C child nodes. If you didn't have any I2C nodes, then you would need to delete the property from the parent node as well. The phandle in the mpc5200 case could be implicit since it is fixed in silicon. If we want to use the same I2C driver for 52xx and 83xx, it shouldn't be implicit. Otherwise, the driver will have to check the platform to determine where to look. Is this register in a system register bank or an i2c one? gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG That should be guts- I think. The global utilities is a block of miscellaneous registers, one per SOC. It's not part of the I2C block. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 8/1/08, Timur Tabi [EMAIL PROTECTED] wrote: On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl [EMAIL PROTECTED] wrote: I've having trouble with whether these clocks are a system resource or something that belongs to i2c. If they are a system resource then we should make nodes in the root and use a phandle in the i2c node to link to them. I can't speak for 52xx, but for 8[356]xx, I would say the clocks belong to the I2C devices. The screwball determination of whether to divide by 1, 2, or 3 only applies to the I2C device only. The divider itself is not used to drive a clock for any other device. If you disable I2C support, then you don't need to care about the divider (or its output clock) at all. That's why I think have the I2C clock in the parent node is wrong, because it would only be used if you had I2C child nodes. If you didn't have any I2C nodes, then you would need to delete the property from the parent node as well. I see this as being one of three ways... The source clocks belong to the platform and the clock messiness belongs in the platform code. The source clocks belong to i2c. The i2c driver needs to use platform specific bindings like Grant proposed. I don't like the third choice. Keep a simple Linux driver for i2c and the platform, and then move all of the messy code into uboot. BTW, the messy code is about 10 lines. It's going to take more than 10 lines to hide those 10 lines. I'm also of the view that all clocks are system resources. Linux is designed to have clock domains, we just aren't using them on PowerPC. Check out arch/powerpc/kernel/clock.c. They are mostly used on ARM. Could we define domains I2C1, I2C2,.. and then implement them? That implies using the root node to communicate the clock speeds to Linux. -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 8/1/08, Timur Tabi [EMAIL PROTECTED] wrote: On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl [EMAIL PROTECTED] wrote: I've having trouble with whether these clocks are a system resource or something that belongs to i2c. If they are a system resource then we should make nodes in the root and use a phandle in the i2c node to link to them. I can't speak for 52xx, but for 8[356]xx, I would say the clocks belong to the I2C devices. The screwball determination of whether to divide by 1, 2, or 3 only applies to the I2C device only. The divider itself is not used to drive a clock for any other device. If you disable I2C support, then you don't need to care about the divider (or its output clock) at all. That's why I think have the I2C clock in the parent node is wrong, because it would only be used if you had I2C child nodes. If you didn't have any I2C nodes, then you would need to delete the property from the parent node as well. I see this as being one of three ways... The source clocks belong to the platform and the clock messiness belongs in the platform code. The source clocks belong to i2c. The i2c driver needs to use platform specific bindings like Grant proposed. I don't like the third choice. Keep a simple Linux driver for i2c and the platform, and then move all of the messy code into uboot. BTW, the messy code is about 10 lines. It's going to take more than 10 lines to hide those 10 lines. I'm also of the view that all clocks are system resources. Linux is designed to have clock domains, we just aren't using them on PowerPC. Check out arch/powerpc/kernel/clock.c. They are mostly used on ARM. Could we define domains I2C1, I2C2,.. and then implement them? That implies using the root node to communicate the clock speeds to Linux. -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 6:46 PM, Trent Piepho [EMAIL PROTECTED] wrote: The i2c controller just uses some system clock that was handy. For each chip, the designers consult tea leaves to choose a system clock at random to connect to the i2c controller. heh; I thought it was the phase of the moon. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, Aug 1, 2008 at 1:25 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Jon Smirl wrote:
On 7/31/08, Trent Piepho [EMAIL PROTECTED] 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 mpc8 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:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
bus-frequency = 10;
/* 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:
That is true; and if pin-strapping/dip-switch settings are changed,
then that too should be described in the device tree. However, as
Trent stated, that still leaves the question of *which* clock is the
divider applied against. If it isn't the bus-frequency, then there
needs to be a way to override it (an optional property would be usable
here).
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.
implementation detail. Get the device tree binding correct first.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, 31 Jul 2008, Trent Piepho wrote: The i2c controller just uses some system clock that was handy. For each chip, the designers consult tea leaves to choose a system clock at random to connect to the i2c controller. Oh, they decided which clock to choose at design time, not at power-on time? With kind regards, Geert Uytterhoeven Software Architect Sony Techsoft Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis 293-0376800-10 GEBA-BE-BB___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: What about the Efika which is mpc5200 based and doesn't use uboot? How does the Efika handle the dozen other properties that U-Boot normally initializes in the device tree? -- Timur Tabi Linux Kernel Developer @ Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, Aug 01, 2008 at 08:17:25AM -0500, Timur Tabi wrote: On Thu, Jul 31, 2008 at 6:32 PM, Trent Piepho [EMAIL PROTECTED] wrote: Sometimes the two i2c controllers don't even have the same clock. On which platform is that the case? I thought I had all 8[356]xx boards covered. Did I miss some? On 8313, i2c1 is driven by the encryption clock (whose divider from the CSB clock is programmable as 1, 2, or 3), but i2c2 is driven directly by the CSB clock. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 8/1/08, Timur Tabi [EMAIL PROTECTED] wrote: Jon Smirl wrote: What about the Efika which is mpc5200 based and doesn't use uboot? How does the Efika handle the dozen other properties that U-Boot normally initializes in the device tree? Efika is like the original OpenFirmware. It has a Forth interpreter and builds the device tree itself. It doesn't use flat device trees that get filled in by the boot firmware. -- Timur Tabi Linux Kernel Developer @ Freescale -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, 1 Aug 2008, Timur Tabi wrote: On Thu, Jul 31, 2008 at 6:32 PM, Trent Piepho [EMAIL PROTECTED] wrote: The real problem, which kept me from making a patch to do this months ago, is that the source clock that the I2C freq divider applies to is different for just about every MPC8xxx platform. Not just a different value, but a totally different internal clock. Sometimes is CCB, sometimes CCB/2, sometimes tsec2's clock, etc. On which SOC is it the tesc2 clock? 834x Sometimes the two i2c controllers don't even have the same clock. On which platform is that the case? I thought I had all 8[356]xx boards covered. Did I miss some? All 83xx other than 832x. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Trent Piepho wrote: All 83xx other than 832x. Never mind, I forgot that 83xx support for i2c1_clk was already in U-Boot: #if defined(CONFIG_MPC834X) i2c1_clk = tsec2_clk; #elif defined(CONFIG_MPC8360) i2c1_clk = csb_clk; #elif defined(CONFIG_MPC832X) i2c1_clk = enc_clk; #elif defined(CONFIG_MPC831X) i2c1_clk = enc_clk; #elif defined(CONFIG_MPC837X) i2c1_clk = sdhc_clk; #endif #if !defined(CONFIG_MPC832X) i2c2_clk = csb_clk; /* i2c-2 clk is equal to csb clk */ #endif -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, 1 Aug 2008, Jon Smirl wrote:
I don't like the third choice. Keep a simple Linux driver for i2c and
the platform, and then move all of the messy code into uboot. BTW,
the messy code is about 10 lines. It's going to take more than 10
lines to hide those 10 lines.
It's not being _moved_ to u-boot, it's already there. U-boot needs it
because u-boot uses i2c.
It would need to be duplicated in linux, and then both u-boot and linux
would need to be updated each time a new platform is added.
It's pretty much a given that a u-boot binary will only work on one
platform. The same is not true with a compiled kernel. That makes it
harder to all the platform specific stuff in linux. It's a little more
than 10 lines too. Keep in mind that accessing all these devices registers
isn't as easy in linux as u-boot. In linux you have to look up the
register location in device tree and map the registers. All this code uses
a system clock as a starting point, which u-boot already passes to linux.
#if defined(CONFIG_MPC83XX)
volatile immap_t *im = (immap_t *) CFG_IMMR;
sccr = im-clk.sccr;
#if defined(CONFIG_MPC834X) || defined(CONFIG_MPC831X) ||
defined(CONFIG_MPC837X)
switch ((sccr SCCR_TSEC1CM) SCCR_TSEC1CM_SHIFT) {
case 0:
tsec1_clk = 0;
break;
case 1:
tsec1_clk = csb_clk;
break;
case 2:
tsec1_clk = csb_clk / 2;
break;
case 3:
tsec1_clk = csb_clk / 3;
break;
default:
/* unkown SCCR_TSEC1CM value */
return -2;
}
#endif
#if defined(CONFIG_MPC834X) || defined(CONFIG_MPC837X) ||
defined(CONFIG_MPC8315)
switch ((sccr SCCR_TSEC2CM) SCCR_TSEC2CM_SHIFT) {
case 0:
tsec2_clk = 0;
break;
case 1:
tsec2_clk = csb_clk;
break;
case 2:
tsec2_clk = csb_clk / 2;
break;
case 3:
tsec2_clk = csb_clk / 3;
break;
default:
/* unkown SCCR_TSEC2CM value */
return -4;
}
#elif defined(CONFIG_MPC8313)
tsec2_clk = tsec1_clk;
if (!(sccr SCCR_TSEC1ON))
tsec1_clk = 0;
if (!(sccr SCCR_TSEC2ON))
tsec2_clk = 0;
#endif
switch ((sccr SCCR_ENCCM) SCCR_ENCCM_SHIFT) {
case 0:
enc_clk = 0;
break;
case 1:
enc_clk = csb_clk;
break;
case 2:
enc_clk = csb_clk / 2;
break;
case 3:
enc_clk = csb_clk / 3;
break;
default:
/* unkown SCCR_ENCCM value */
return -7;
}
#if defined(CONFIG_MPC837X)
switch ((sccr SCCR_SDHCCM) SCCR_SDHCCM_SHIFT) {
case 0:
sdhc_clk = 0;
break;
case 1:
sdhc_clk = csb_clk;
break;
case 2:
sdhc_clk = csb_clk / 2;
break;
case 3:
sdhc_clk = csb_clk / 3;
break;
default:
/* unkown SCCR_SDHCCM value */
return -8;
}
#endif
#if defined(CONFIG_MPC834X)
i2c1_clk = tsec2_clk;
#elif defined(CONFIG_MPC8360)
i2c1_clk = csb_clk;
#elif defined(CONFIG_MPC832X)
i2c1_clk = enc_clk;
#elif defined(CONFIG_MPC831X)
i2c1_clk = enc_clk;
#elif defined(CONFIG_MPC837X)
i2c1_clk = sdhc_clk;
#endif
#if !defined(CONFIG_MPC832X)
i2c2_clk = csb_clk; /* i2c-2 clk is equal to csb clk */
#endif
#elif defined (CONFIG_MPC85XX)
#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
gd-i2c1_clk = sys_info.freqSystemBus;
#elif defined(CONFIG_MPC8544)
volatile ccsr_gur_t *gur = (void *) CFG_MPC85xx_GUTS_ADDR;
/*
* On the 8544, the I2C clock is the same as the SEC clock. This can be
* either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See
* 4.4.3.3 of the 8544 RM. Note that this might actually work for all
* 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the
* PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544.
*/
if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG)
gd-i2c1_clk = sys_info.freqSystemBus / 3;
else
gd-i2c1_clk = sys_info.freqSystemBus / 2;
#else
/* Most 85xx SOCs use CCB/2, so this is the default behavior. */
gd-i2c1_clk = sys_info.freqSystemBus / 2;
#endif
gd-i2c2_clk = gd-i2c1_clk;
#elif defined(CONFIG_MPC86XX)
#ifdef CONFIG_MPC8610
gd-i2c1_clk = sys_info.freqSystemBus;
#else
gd-i2c1_clk = sys_info.freqSystemBus /
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, 31 Jul 2008, Timur Tabi wrote: Jon Smirl wrote: Aren't the tables in the manual there just to make it easy for a human to pick out the line they want? For a computer you'd program the formula that was used to create the tables. Actually, the tables in the manuals are just an example of what can be programmed. They don't cover all cases. The tables assume a specific value of DFSR. For 8xxx, you want to use AN2919 to figure out how to really program the device. The table I generated for U-Boot is based on the calculations outlined in AN2919. I debated trying to implement the actual algorithm, but decided that pre-calculating the values was better. The algorithm is very convoluted. And I decided to program the algorithm. It's not that complex and overall compiles to less total size than the table method. It doesn't involve some black box table either. The real problem, which kept me from making a patch to do this months ago, is that the source clock that the I2C freq divider applies to is different for just about every MPC8xxx platform. Not just a different value, but a totally different internal clock. Sometimes is CCB, sometimes CCB/2, sometimes tsec2's clock, etc. Sometimes the two i2c controllers don't even have the same clock. I didn't see any easy way to get this information. In U-Boot I get it from the global board info struct, but I didn't see anything like this in Linux. I agree that it took me half an hour to figure out the formula that was needed to compute the i2s clocks, but once you figure out the formula it solves all of the cases and no one needs to read the manual any more. The i2c formula may even need a small loop which compares different solutions looking for the smallest error term. But it's a small space to search. Yes, I needed a loop. The divider ends up having the form (a + c) b, where a is from some set of eight integers between 2 and 15, b has a range of like 5 to 12, and c is usually zero. So, I find the divider I want to get, loop over each b, shift the divider right by b and find the closest (a+c) to that, calculate the error, and then use the best one. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
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 mpc8 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:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
bus-frequency = 10;
/* Either */
source-clock-frequency = 0;
/* OR */
source-clock = ccb;
};
bus-frequency is the desired frequency of the i2c bus, i.e. 100 kHz or 400
kHz usually. source-clock-frequency is the the source clock to this i2c
controller. U-Boot can fill this in since it already knows it anyway. Or,
instead of source-clock-frequency, source-clock can be specified as a
phandle to a device which has the clock to use. This could be useful if
Linux can change the clock frequency.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, 31 Jul 2008, Grant Likely wrote:
On Thu, Jul 31, 2008 at 09:54:48PM +0200, Wolfgang Grandegger wrote:
Thinking more about it, it would be best to add the property
i2c-clock-divider to the soc node and implement fsl_get_i2c_freq() in
a similar way:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 1;
device_type = soc;
ranges = 0x0 0xe000 0x10;
reg = 0xe000 0x1000; // CCSRBAR 1M
bus-frequency = 0;
i2c-clock-divider = 2;
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
Except the i2c clock isn't always a based on an interger 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?
Ugh. This is specifically related to the i2c device, so please place
the property in the i2c device. Remember, device tree design is not
about what will make the implementation simplest, but rather about what
describes the hardware in the best way.
Now, if you can argue that i2c-clock-frequency is actually a separate
clock domain defined at the SoC level, not the i2c device level, then I
will change my opinion.
The i2c controller just uses some system clock that was handy. For each
chip, the designers consult tea leaves to choose a system clock at random
to connect to the i2c controller.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, 31 Jul 2008, Grant Likely wrote: I'm a bit confused. The frequency of the I2C source clock and the real I2C clock frequency are two different things. The first one is common for all I2C devices, the second can be different. What properties would you like to use for defining both? How is the divider controlled? Is it a fixed property of the SoC? Usually. a shared register setting? Sometimes. or a register setting within the i2c device? Never. Thinking of it as the CCB divided by 1, 2, 3 isn't really right. It's just some internal clock that I2C was connected to. Sometimes it's the clock for the ethernet block of the SoC. Sometimes it comes from some other block. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, 31 Jul 2008, Timur Tabi wrote: Wolfgang Grandegger wrote: U-Boot does not (yet) use the FDT and it has therefore to use that ugly code to derive the I2C input clock frequency. I think its completely legal to put that hardware specific information into the FDT and get rid of such code. Huh? U-Boot initializes several fields and creates several properties in the FDT today, so what's wrong with adding another one? The clock frequencies have always been calculated by U-Boot, because putting them in the device tree is a bad idea. I think Wolfgang means u-boot doesn't _read_ information from the device tree. Put the I2C source clock into the device tree and have u-boot read it in. But that simply doesn't work at all. The i2c source clock isn't constant. It's not a constant divider from the core clock either. Flip a dip switch, and the divider changes. But that's irrelevant. U-boot needs to configure the i2c controller to read SPD data from the DIMM's EPROM, which is necessary to get DRAM working. That happens long long before the FDT is loaded via TFTP, y-modem, NFS, or from disk or flash. Might as well have u-boot get the i2c source clock via Linux's sysfs. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Trent Piepho [EMAIL PROTECTED] 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 mpc8 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:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
bus-frequency = 10;
/* 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).
I don't see why we want to go through the trouble of having uboot tell
us things about a chip that are fixed in stone. Obviously something
like the frequency of the external crystal needs to be passed up, but
why pass up stuff that can be computed (or recovered by reading a
register)?
I may be using uboot differently, I just use it to boot and removed
support for everything else.
bus-frequency is the desired frequency of the i2c bus, i.e. 100 kHz or 400
kHz usually. source-clock-frequency is the the source clock to this i2c
controller. U-Boot can fill this in since it already knows it anyway. Or,
instead of source-clock-frequency, source-clock can be specified as a
phandle to a device which has the clock to use. This could be useful if
Linux can change the clock frequency.
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, 31 Jul 2008, Jon Smirl wrote:
Here's my idea:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
bus-frequency = 10;
/* 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.
If you have to support clock speed in the i2c node anyway, what's the point
of the other options?
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).
You need a bunch of random other device registers (SEC, ethernet, sdhc,
etc.) too.
I don't see why we want to go through the trouble of having uboot tell
us things about a chip that are fixed in stone. Obviously something
like the frequency of the external crystal needs to be passed up, but
why pass up stuff that can be computed (or recovered by reading a
register)?
One could also say that if U-boot has to have the code and already
calculated the value, why duplicate the code and the calculation in Linux?
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Trent Piepho [EMAIL PROTECTED] wrote:
On Thu, 31 Jul 2008, Jon Smirl wrote:
Here's my idea:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
bus-frequency = 10;
/* 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.
If you have to support clock speed in the i2c node anyway, what's the point
of the other options?
So that I don't have to change my existing mpc5200 systems. mpc5200
has no need for specifying the source clock in each i2c node, hardware
doesn't allow it.
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).
You need a bunch of random other device registers (SEC, ethernet, sdhc,
etc.) too.
I don't see why we want to go through the trouble of having uboot tell
us things about a chip that are fixed in stone. Obviously something
like the frequency of the external crystal needs to be passed up, but
why pass up stuff that can be computed (or recovered by reading a
register)?
One could also say that if U-boot has to have the code and already
calculated the value, why duplicate the code and the calculation in Linux?
What about the Efika which is mpc5200 based and doesn't use uboot?
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 09:19:51PM -0400, Jon Smirl wrote:
On 7/31/08, Trent Piepho [EMAIL PROTECTED] 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 mpc8 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:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
bus-frequency = 10;
/* 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.
Your mixing up device tree layout with implementation details. Device
tree layout must come first. mpc52xx_find_ipb_freq() is just a
convenience function that walks up the device tree looking for a
bus-frequency property.
So, instead of making arguments based on available helper functions,
make your arguments based on how data should be laid out in the device
tree. Currently mpc5200 bindings simply depend on finding a
bus-frequency property in the parent node for determining the input
clock and I don't see any pressing reason to change that (though it
probably needs to be documented better).
However, for the complex cases that Trent and Timur are talking about,
it makes perfect sense to have an optional property in the i2c node
itself that defines a different clock. Once that decision has been made
and documented, then the driver can be modified and the appropriate
helper functions added to adapt the device tree data into something
useful.
Remember (and chant this to yourself). The device tree describes
*hardware*. It does not describe Linux internal implementation details.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Grant Likely [EMAIL PROTECTED] wrote: Your mixing up device tree layout with implementation details. Device tree layout must come first. mpc52xx_find_ipb_freq() is just a convenience function that walks up the device tree looking for a bus-frequency property. So, instead of making arguments based on available helper functions, make your arguments based on how data should be laid out in the device tree. Currently mpc5200 bindings simply depend on finding a bus-frequency property in the parent node for determining the input clock and I don't see any pressing reason to change that (though it probably needs to be documented better). However, for the complex cases that Trent and Timur are talking about, it makes perfect sense to have an optional property in the i2c node itself that defines a different clock. Once that decision has been made and documented, then the driver can be modified and the appropriate helper functions added to adapt the device tree data into something useful. I've having trouble with whether these clocks are a system resource or something that belongs to i2c. If they are a system resource then we should make nodes in the root and use a phandle in the i2c node to link to them. The phandle in the mpc5200 case could be implicit since it is fixed in silicon. Is this register in a system register bank or an i2c one? gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG Remember (and chant this to yourself). The device tree describes *hardware*. It does not describe Linux internal implementation details. g. -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
