From: Wolfgang Grandegger <hidden> Date: 2008-07-25 07:37:25
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties "fdr" and "dfsrr" for the
corresponding I2C registers to make the speed configurable via FDT,
e.g.:
i2c@3100 {
compatible = "fsl-i2c";
reg = <0x3100 0x100>;
interrupts = <43 2>;
interrupt-parent = <&mpic>;
dfsrr = <0x20>;
fdr = <0x03>;
};
Signed-off-by: Wolfgang Grandegger <redacted>
---
Documentation/powerpc/dts-bindings/fsl/i2c.txt | 9 +++--
drivers/i2c/busses/i2c-mpc.c | 45 ++++++++++++++++++++-----
2 files changed, 42 insertions(+), 12 deletions(-)
Index: linux-2.6-galak/drivers/i2c/busses/i2c-mpc.c
===================================================================
@@ -156,13 +158,16 @@ static int i2c_wait(struct mpc_i2c *i2c,staticvoidmpc_i2c_setclock(structmpc_i2c*i2c){/* Set clock and filters */+pr_debug("I2C: flags=%#x fdr=%#x dfsrr=%#x\n",+i2c->flags,i2c->fdr,i2c->dfsrr);if(i2c->flags&FSL_I2C_DEV_SEPARATE_DFSRR){-writeb(0x31,i2c->base+MPC_I2C_FDR);-writeb(0x10,i2c->base+MPC_I2C_DFSRR);-}elseif(i2c->flags&FSL_I2C_DEV_CLOCK_5200)-writeb(0x3f,i2c->base+MPC_I2C_FDR);-else-writel(0x1031,i2c->base+MPC_I2C_FDR);+writeb(i2c->fdr,i2c->base+MPC_I2C_FDR);+writeb(i2c->dfsrr,i2c->base+MPC_I2C_DFSRR);+}elseif(i2c->flags&FSL_I2C_DEV_CLOCK_5200){+writeb(i2c->fdr,i2c->base+MPC_I2C_FDR);+}else{+writel(i2c->fdr,i2c->base+MPC_I2C_FDR);+}}staticvoidmpc_i2c_start(structmpc_i2c*i2c)
@@ -320,18 +325,40 @@ static int __devinit fsl_i2c_probe(struc{intresult=0;structmpc_i2c*i2c;+constu32*prop;+intprop_len;i2c=kzalloc(sizeof(*i2c),GFP_KERNEL);if(!i2c)return-ENOMEM;-if(of_get_property(op->node,"dfsrr",NULL))-i2c->flags|=FSL_I2C_DEV_SEPARATE_DFSRR;-if(of_device_is_compatible(op->node,"fsl,mpc5200-i2c")||of_device_is_compatible(op->node,"mpc5200-i2c"))i2c->flags|=FSL_I2C_DEV_CLOCK_5200;+prop=of_get_property(op->node,"dfsrr",&prop_len);+if(prop){+i2c->flags|=FSL_I2C_DEV_SEPARATE_DFSRR;+if(prop_len==sizeof(*prop))+i2c->dfsrr=*prop;+else+/* Set resonable default value */+i2c->dfsrr=0x10;+}++prop=of_get_property(op->node,"fdr",&prop_len);+if(prop&&prop_len==sizeof(*prop)){+i2c->fdr=*prop;+}else{+/* Set resonable default values */+if(i2c->flags&FSL_I2C_DEV_SEPARATE_DFSRR)+i2c->fdr=0x31;+elseif(i2c->flags&FSL_I2C_DEV_CLOCK_5200)+i2c->fdr=0x3f;+else+i2c->fdr=0x1031;+}+init_waitqueue_head(&i2c->queue);i2c->base=of_iomap(op->node,0);
@@ -16,10 +16,12 @@ Recommended properties : controller you have. - interrupt-parent : the phandle for the interrupt controller that services interrupts for this device.- - dfsrr : boolean; if defined, indicates that this I2C device has- a digital filter sampling rate register - fsl5200-clocking : boolean; if defined, indicated that this device uses the FSL 5200 clocking mechanism.+ - dfsrr : boolean or <v>; if defined, indicates that this I2C device has+ a digital filter sampling rate register. Optionally you can specify a+ value v for the this register.+ - fdr : <v>, if defined, the FDR register will be set to the value v. Example : i2c@3000 {
From: Jochen Friedrich <jochen@scram.de> Date: 2008-07-25 08:51:56
Hi Wolfgang,
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties "fdr" and "dfsrr" for the
corresponding I2C registers to make the speed configurable via FDT,
e.g.:
i2c@3100 {
compatible = "fsl-i2c";
reg = <0x3100 0x100>;
interrupts = <43 2>;
interrupt-parent = <&mpic>;
dfsrr = <0x20>;
fdr = <0x03>;
};
Would it be possible to use the standard property "clock-frequency" for this
and calculate the register settings in the driver?
Thanks,
Jochen
From: Wolfgang Grandegger <hidden> Date: 2008-07-25 09:04:54
Jochen Friedrich wrote:
Hi Wolfgang,
quoted
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties "fdr" and "dfsrr" for the
corresponding I2C registers to make the speed configurable via FDT,
e.g.:
i2c@3100 {
compatible = "fsl-i2c";
reg = <0x3100 0x100>;
interrupts = <43 2>;
interrupt-parent = <&mpic>;
dfsrr = <0x20>;
fdr = <0x03>;
};
Would it be possible to use the standard property "clock-frequency" for this
and calculate the register settings in the driver?
Almost everything is possible in software, just for what price ;-).
U-Boot has some code in drivers/i2c/fsl_i2c.c to determine reasonable
fdr and dfsrr values for the MPC83/5/6xx boards. For the MPC82xx and
MPC85xx it's even more sophisticated.
I was also thinking to just overtake the U-Boot settings if fdt and
dfsrr is not defined for the I2C node (instead of the debatable default
values).
Wolfgang.
From: Grant Likely <hidden> Date: 2008-07-25 13:12:10
On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger [off-list ref] wrote:
Jochen Friedrich wrote:
quoted
Hi Wolfgang,
quoted
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties "fdr" and "dfsrr" for the
corresponding I2C registers to make the speed configurable via FDT, e.g.:
i2c@3100 {
compatible = "fsl-i2c";
reg = <0x3100 0x100>;
interrupts = <43 2>;
interrupt-parent = <&mpic>;
dfsrr = <0x20>;
fdr = <0x03>;
};
Would it be possible to use the standard property "clock-frequency" for
this
and calculate the register settings in the driver?
Yes, please use something like clock-frequency or current-speed and do
the calculation.
Almost everything is possible in software, just for what price ;-). U-Boot
has some code in drivers/i2c/fsl_i2c.c to determine reasonable fdr and dfsrr
values for the MPC83/5/6xx boards. For the MPC82xx and MPC85xx it's even
more sophisticated.
I was also thinking to just overtake the U-Boot settings if fdt and dfsrr is
not defined for the I2C node (instead of the debatable default values).
This is a perfectly valid option. Personally, I'd prefer it encoded
in the device tree, but if it looks like a valid speed has already
been programmed in then I'm cool with the driver just preserving that.
If it turns out to causes problems the we can always change the code
to be more conservative later.
Thanks,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
From: Timur Tabi <hidden> Date: 2008-07-25 14:21:31
On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely [off-list ref] wrote:
Yes, please use something like clock-frequency or current-speed and do
the calculation.
Ditto. I already wrote the code that does that for U-Boot, so all you
need to do is port it.
Although I'm curious, if U-Boot already programs the speed, why does
the driver program it again?
--
Timur Tabi
Linux kernel developer at Freescale
From: Jon Smirl <hidden> Date: 2008-07-25 15:04:12
On 7/25/08, Timur Tabi [off-list ref] wrote:
On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely [off-list ref] wrote:
> Yes, please use something like clock-frequency or current-speed and do
> the calculation.
Ditto. I already wrote the code that does that for U-Boot, so all you
need to do is port it.
I calculate the register values in the i2s driver. There is the issue
of requesting a frequency the hardware can't make exactly (Freescale -
more FractionalN dividers please!).
if (dir == SND_SOC_CLOCK_OUT) {
psc_i2s->sysclk = freq;
if (clk_id == MPC52xx_CLK_CELLSLAVE) {
psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE | MPC52xx_PSC_SICR_GENCLK;
} else { /* MPC52xx_CLK_INTERNAL */
psc_i2s->sicr &= ~MPC52xx_PSC_SICR_CELLSLAVE;
psc_i2s->sicr |= MPC52xx_PSC_SICR_GENCLK;
clkdiv = ppc_proc_freq / freq;
err = ppc_proc_freq % freq;
if (err > freq / 2)
clkdiv++;
dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(clkdiv %d freq error=%ldHz)\n",
clkdiv, (ppc_proc_freq / clkdiv - freq));
return mpc52xx_set_psc_clkdiv(psc_i2s->dai.id + 1, clkdiv);
}
}
if (psc_i2s->sysclk) {
framesync = bits * 2;
bitclk = (psc_i2s->sysclk) / (params_rate(params) * framesync);
/* bitclk field is byte swapped due to mpc5200/b compatibility */
value = ((framesync - 1) << 24) |
(((bitclk - 1) & 0xFF) << 16) | ((bitclk - 1) & 0xFF00);
dev_dbg(psc_i2s->dev, "%s(substream=%p) rate=%i sysclk=%i"
" framesync=%i bitclk=%i reg=%X\n",
__FUNCTION__, substream, params_rate(params), psc_i2s->sysclk,
framesync, bitclk, value);
out_be32(&psc_i2s->psc_regs->ccr, value);
out_8(&psc_i2s->psc_regs->ctur, bits - 1);
}
--
Jon Smirl
jonsmirl@gmail.com
From: Wolfgang Grandegger <hidden> Date: 2008-07-25 15:34:49
Grant Likely wrote:
On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger [off-list ref] wrote:
quoted
Jochen Friedrich wrote:
quoted
Hi Wolfgang,
quoted
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties "fdr" and "dfsrr" for the
corresponding I2C registers to make the speed configurable via FDT, e.g.:
i2c@3100 {
compatible = "fsl-i2c";
reg = <0x3100 0x100>;
interrupts = <43 2>;
interrupt-parent = <&mpic>;
dfsrr = <0x20>;
fdr = <0x03>;
};
Would it be possible to use the standard property "clock-frequency" for
this
and calculate the register settings in the driver?
Yes, please use something like clock-frequency or current-speed and do
the calculation.
quoted
Almost everything is possible in software, just for what price ;-). U-Boot
has some code in drivers/i2c/fsl_i2c.c to determine reasonable fdr and dfsrr
values for the MPC83/5/6xx boards. For the MPC82xx and MPC85xx it's even
more sophisticated.
I was also thinking to just overtake the U-Boot settings if fdt and dfsrr is
not defined for the I2C node (instead of the debatable default values).
This is a perfectly valid option. Personally, I'd prefer it encoded
in the device tree, but if it looks like a valid speed has already
been programmed in then I'm cool with the driver just preserving that.
If it turns out to causes problems the we can always change the code
to be more conservative later.
How should the Linux driver decide if the registers have been already
set by the boot-loader? The reset-values might be good as well.
Therefore, if "clock-frequency" is not specified, the driver may simply
not touch the fdr and dfsr registers (overtaking the values from the
boot-loader).
What do you think.
Wolfgang.
From: Timur Tabi <hidden> Date: 2008-07-25 16:19:41
Wolfgang Grandegger wrote:
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the device
tree is not a good way to do this.
--
Timur Tabi
Linux kernel developer at Freescale
From: Grant Likely <hidden> Date: 2008-07-27 01:25:47
On Fri, Jul 25, 2008 at 05:34:49PM +0200, Wolfgang Grandegger wrote:
Grant Likely wrote:
quoted
On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger [off-list ref] wrote:
quoted
I was also thinking to just overtake the U-Boot settings if fdt and dfsrr is
not defined for the I2C node (instead of the debatable default values).
This is a perfectly valid option. Personally, I'd prefer it encoded
in the device tree, but if it looks like a valid speed has already
been programmed in then I'm cool with the driver just preserving that.
If it turns out to causes problems the we can always change the code
to be more conservative later.
How should the Linux driver decide if the registers have been already
set by the boot-loader? The reset-values might be good as well.
Therefore, if "clock-frequency" is not specified, the driver may simply
not touch the fdr and dfsr registers (overtaking the values from the
boot-loader).
I'm okay with that. If the property isn't present then it is probably
okay to just assume that it is usable.
g.
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 11:51:32
Grant Likely wrote:
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
quoted
Wolfgang Grandegger wrote:
quoted
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the device
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We
would need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
- furthermore, there are various mpc-specific I2C clock sources:
MPC82xx : fsl_get_sys_freq()
MPC5200 : IPB
MPC83xx : fsl_get_sys_freq()
MPC8540/41/60/55,MPC8610 : fsl_get_sys_freq()
MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2
MPC8544 : fsl_get_sys_freq()/2 or /3
It would make sense to hand-over the I2C frequency from U-Boot to
Linux.
Wolfgang.
From: Jon Smirl <hidden> Date: 2008-07-31 15:49:32
On 7/31/08, Wolfgang Grandegger [off-list ref] wrote:
Grant Likely wrote:
quoted
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
quoted
Wolfgang Grandegger wrote:
quoted
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the
device
quoted
quoted
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
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.
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.
These device tree flags should be removed, the driver can ask the
platform code what CPU it is running on.
if (of_get_property(op->node, "dfsrr", NULL))
i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
if (of_device_is_compatible(op->node, "fsl,mpc5200-i2c") ||
of_device_is_compatible(op->node, "mpc5200-i2c"))
i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
static void mpc_i2c_setclock(struct mpc_i2c *i2c)
{
/* Set clock and filters */
if (i2c->flags & FSL_I2C_DEV_SEPARATE_DFSRR) {
writeb(0x31, i2c->base + MPC_I2C_FDR);
writeb(0x10, i2c->base + MPC_I2C_DFSRR);
} else if (i2c->flags & FSL_I2C_DEV_CLOCK_5200)
writeb(0x3f, i2c->base + MPC_I2C_FDR);
else
writel(0x1031, i2c->base + MPC_I2C_FDR);
}
These defines shouldn't be here, they should get the offset from the
right header file for the CPU. But it appears that structures for the
i2c memory map haven't been done for the various CPUs.
#define MPC_I2C_FDR 0x04
#define MPC_I2C_CR 0x08
#define MPC_I2C_SR 0x0c
#define MPC_I2C_DR 0x10
#define MPC_I2C_DFSRR 0x14
There appears to be one for i2x8xxx but not the other CPUs.
/* I2C
*/
typedef struct i2c {
u_char i2c_i2mod;
char res1[3];
u_char i2c_i2add;
char res2[3];
u_char i2c_i2brg;
char res3[3];
u_char i2c_i2com;
char res4[3];
u_char i2c_i2cer;
char res5[3];
u_char i2c_i2cmr;
char res6[0x8b];
} i2c8xx_t;
- furthermore, there are various mpc-specific I2C clock sources:
MPC82xx : fsl_get_sys_freq()
MPC5200 : IPB
MPC83xx : fsl_get_sys_freq()
MPC8540/41/60/55,MPC8610 : fsl_get_sys_freq()
MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2
MPC8544 : fsl_get_sys_freq()/2 or /3
It would make sense to hand-over the I2C frequency from U-Boot to
Linux.
Wolfgang.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
From: Timur Tabi <hidden> Date: 2008-07-31 15:55:22
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.
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.
My understanding is that the algorithm itself is different on 8xxx vs. 52xx. It
might be possible to combine 5200A and 5200B into one table/algorithm, but I
don't think you can combine it with the 8xxx table.
--
Timur Tabi
Linux kernel developer at Freescale
From: Grant Likely <hidden> Date: 2008-07-31 16:51:30
On Thu, Jul 31, 2008 at 5:51 AM, Wolfgang Grandegger [off-list ref] wrote:
Grant Likely wrote:
quoted
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
quoted
Wolfgang Grandegger wrote:
quoted
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the
device
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
- furthermore, there are various mpc-specific I2C clock sources:
MPC82xx : fsl_get_sys_freq()
MPC5200 : IPB
MPC83xx : fsl_get_sys_freq()
MPC8540/41/60/55,MPC8610 : fsl_get_sys_freq()
MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2
MPC8544 : fsl_get_sys_freq()/2 or /3
It would make sense to hand-over the I2C frequency from U-Boot to
Linux.
U-Boot isn't always available and there are plenty of 5200 and 8xxx
boards out there which will never have U-Boot reflashed to provide
this data. Also, there are boards that don't even use U-Boot. I
don't want to go down this path. It is the drivers *job* to
understand how to set these registers.
If you're careful, the table doesn't need to be huge. It can be
marked as initdata and conditionally compiled depending on which
architectures are compiled in. You should use .data in the driver's
of_device_id table to provide machine specific ops for setting
clocking to avoid a maze of if/else statements.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
From: Jon Smirl <hidden> Date: 2008-07-31 17:06:41
On 7/31/08, Grant Likely [off-list ref] wrote:
If you're careful, the table doesn't need to be huge. It can be
marked as initdata and conditionally compiled depending on which
architectures are compiled in. You should use .data in the driver's
of_device_id table to provide machine specific ops for setting
clocking to avoid a maze of if/else statements.
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 17:22:32
Jon Smirl wrote:
On 7/31/08, Wolfgang Grandegger [off-list ref] wrote:
quoted
Grant Likely wrote:
quoted
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
quoted
Wolfgang Grandegger wrote:
quoted
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the
device
quoted
quoted
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
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.
I have the formulas to create the tables, also for the MPC5200 Rev. A
and B. That was not my point. I'm worried about arch specific code in
i2c-mpc.c. It should go somewhere to arch/powerpc.
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.
These device tree flags should be removed, the driver can ask the
platform code what CPU it is running on.
if (of_get_property(op->node, "dfsrr", NULL))
i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
if (of_device_is_compatible(op->node, "fsl,mpc5200-i2c") ||
of_device_is_compatible(op->node, "mpc5200-i2c"))
i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
static void mpc_i2c_setclock(struct mpc_i2c *i2c)
{
/* Set clock and filters */
if (i2c->flags & FSL_I2C_DEV_SEPARATE_DFSRR) {
writeb(0x31, i2c->base + MPC_I2C_FDR);
writeb(0x10, i2c->base + MPC_I2C_DFSRR);
} else if (i2c->flags & FSL_I2C_DEV_CLOCK_5200)
writeb(0x3f, i2c->base + MPC_I2C_FDR);
else
writel(0x1031, i2c->base + MPC_I2C_FDR);
}
These defines shouldn't be here, they should get the offset from the
right header file for the CPU. But it appears that structures for the
i2c memory map haven't been done for the various CPUs.
#define MPC_I2C_FDR 0x04
#define MPC_I2C_CR 0x08
#define MPC_I2C_SR 0x0c
#define MPC_I2C_DR 0x10
#define MPC_I2C_DFSRR 0x14
There appears to be one for i2x8xxx but not the other CPUs.
/* I2C
*/
typedef struct i2c {
u_char i2c_i2mod;
char res1[3];
u_char i2c_i2add;
char res2[3];
u_char i2c_i2brg;
char res3[3];
u_char i2c_i2com;
char res4[3];
u_char i2c_i2cer;
char res5[3];
u_char i2c_i2cmr;
char res6[0x8b];
} i2c8xx_t;
The I2C interface for the MPC5200 is not compatible with the one for the
MPC83/4/5/6xx, AFAIK.
Wolfgang.
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 17:24:25
Grant Likely wrote:
On Thu, Jul 31, 2008 at 5:51 AM, Wolfgang Grandegger [off-list ref] wrote:
quoted
Grant Likely wrote:
quoted
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
quoted
Wolfgang Grandegger wrote:
quoted
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the
device
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
- furthermore, there are various mpc-specific I2C clock sources:
MPC82xx : fsl_get_sys_freq()
MPC5200 : IPB
MPC83xx : fsl_get_sys_freq()
MPC8540/41/60/55,MPC8610 : fsl_get_sys_freq()
MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2
MPC8544 : fsl_get_sys_freq()/2 or /3
It would make sense to hand-over the I2C frequency from U-Boot to
Linux.
U-Boot isn't always available and there are plenty of 5200 and 8xxx
boards out there which will never have U-Boot reflashed to provide
this data. Also, there are boards that don't even use U-Boot. I
don't want to go down this path. It is the drivers *job* to
understand how to set these registers.
If you're careful, the table doesn't need to be huge. It can be
marked as initdata and conditionally compiled depending on which
architectures are compiled in. You should use .data in the driver's
of_device_id table to provide machine specific ops for setting
clocking to avoid a maze of if/else statements.
From: Grant Likely <hidden> Date: 2008-07-31 17:31:44
On Thu, Jul 31, 2008 at 11:22 AM, Wolfgang Grandegger [off-list ref] wrote:
Jon Smirl wrote:
quoted
On 7/31/08, Wolfgang Grandegger [off-list ref] wrote:
quoted
Grant Likely wrote:
quoted
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
quoted
Wolfgang Grandegger wrote:
quoted
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in
the
device
quoted
quoted
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We
would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
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.
I have the formulas to create the tables, also for the MPC5200 Rev. A and B.
Oh, hey, even better.
That was not my point. I'm worried about arch specific code in i2c-mpc.c. It
should go somewhere to arch/powerpc.
i2c-mpc *is* arch specific. I really don't think you need to worry
about adding a block of code for each supported SoC family. Just
change the of_match table to look something like this:
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = "fsl,mpc5200b-i2c", .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = "fsl,mpc5200-i2c", .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = "fsl,mpc8260-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8349-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8540-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8543-i2c", .data = fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = "fsl,mpc8544-i2c", .data = fsl_i2c_mpc8xxx_div3_set_freq, },
/* keep this only for older device trees with some support
code to figure out
what .data should have pointed to. */
{.compatible = "fsl-i2c", },
{},
};
MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
From: Jon Smirl <hidden> Date: 2008-07-31 17:35:58
On 7/31/08, Wolfgang Grandegger [off-list ref] wrote:
Jon Smirl wrote:
quoted
There appears to be one for i2x8xxx but not the other CPUs.
/* I2C
*/
typedef struct i2c {
u_char i2c_i2mod;
char res1[3];
u_char i2c_i2add;
char res2[3];
u_char i2c_i2brg;
char res3[3];
u_char i2c_i2com;
char res4[3];
u_char i2c_i2cer;
char res5[3];
u_char i2c_i2cmr;
char res6[0x8b];
} i2c8xx_t;
The I2C interface for the MPC5200 is not compatible with the one for the
MPC83/4/5/6xx, AFAIK.
Ignore that table, I mistook MPC8xx for MPC8xxx. That is a MPC8xx table.
From: Grant Likely <hidden> Date: 2008-07-31 17:36:06
On Thu, Jul 31, 2008 at 11:06 AM, Jon Smirl [off-list ref] wrote:
On 7/31/08, Grant Likely [off-list ref] wrote:
quoted
If you're careful, the table doesn't need to be huge. It can be
marked as initdata and conditionally compiled depending on which
architectures are compiled in. You should use .data in the driver's
of_device_id table to provide machine specific ops for setting
clocking to avoid a maze of if/else statements.
Ugh. I hate all the registers defined in structures thing done for
5200, but I guess it is better to stick with established convention
than do it differently.
Yes, I think this is okay (but I haven't double checked the values; I
trust you).
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
From: Jon Smirl <hidden> Date: 2008-07-31 17:47:44
On 7/31/08, Grant Likely [off-list ref] wrote:
On Thu, Jul 31, 2008 at 11:06 AM, Jon Smirl [off-list ref] wrote:
> On 7/31/08, Grant Likely [off-list ref] wrote:
>> If you're careful, the table doesn't need to be huge. It can be
>> marked as initdata and conditionally compiled depending on which
>> architectures are compiled in. You should use .data in the driver's
>> of_device_id table to provide machine specific ops for setting
>> clocking to avoid a maze of if/else statements.
>
> Does this look ok for the mpc5200 i2c struct?
>
> /* I2C Registers */
> struct mpc52xx_i2c {
> u8 madr; /* I2C + 0x00 */
> u8 reserved1[3]; /* I2C + 0x01 */
> u8 mfdr; /* I2C + 0x04 */
> u8 reserved2[3]; /* I2C + 0x05 */
> u8 mcr; /* I2C + 0x08 */
> u8 reserved3[3]; /* I2C + 0x09 */
> u8 msr; /* I2C + 0x0c */
> u8 reserved4[3]; /* I2C + 0x0d */
> u8 mdr; /* I2C + 0x10 */
> u8 reserved5[15]; /* I2C + 0x11 */
> u8 interrupt; /* I2C + 0x20 */
> u8 reserved6[3]; /* I2C + 0x21 */
> u8 mifr; /* I2C + 0x24 */
> };
Ugh. I hate all the registers defined in structures thing done for
5200, but I guess it is better to stick with established convention
than do it differently.
Isn't it better than adding random numbers to a pointer and having to
worry about what the pointer is pointing at so that it will multiply
by the right word size? That's a mess when the registers are different
lengths.
A common i2c struct might be a better idea that adds in the dfsrr
register might be better.
You can set the flags for dfsrr use in these set_freq routines too
since they select on CPU type.
{.compatible = "fsl,mpc5200b-i2c", .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = "fsl,mpc5200-i2c", .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = "fsl,mpc8260-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8349-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8540-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8543-i2c", .data =
fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = "fsl,mpc8544-i2c", .data =
fsl_i2c_mpc8xxx_div3_set_freq, },
Yes, I think this is okay (but I haven't double checked the values; I
trust you).
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 17:51:48
Grant Likely wrote:
On Thu, Jul 31, 2008 at 11:22 AM, Wolfgang Grandegger [off-list ref] wrote:
quoted
Jon Smirl wrote:
quoted
On 7/31/08, Wolfgang Grandegger [off-list ref] wrote:
quoted
Grant Likely wrote:
quoted
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
quoted
Wolfgang Grandegger wrote:
quoted
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in
the
device
quoted
quoted
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We
would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
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.
I have the formulas to create the tables, also for the MPC5200 Rev. A and B.
Oh, hey, even better.
quoted
That was not my point. I'm worried about arch specific code in i2c-mpc.c. It
should go somewhere to arch/powerpc.
i2c-mpc *is* arch specific. I really don't think you need to worry
about adding a block of code for each supported SoC family. Just
change the of_match table to look something like this:
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = "fsl,mpc5200b-i2c", .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = "fsl,mpc5200-i2c", .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = "fsl,mpc8260-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8349-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8540-i2c", .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = "fsl,mpc8543-i2c", .data = fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = "fsl,mpc8544-i2c", .data = fsl_i2c_mpc8xxx_div3_set_freq, },
/* keep this only for older device trees with some support
code to figure out
what .data should have pointed to. */
{.compatible = "fsl-i2c", },
{},
};
MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
Cool, this would also make the "dfsrr" property obsolete. Just the
MPC8544 needs more attention because the I2C clock can be programmed to
be freq/2 or freq/3.
Wolfgang.
So we need to update this table every time there's a new SOC? All 83xx, 85xx,
and 86xx SOCs use the same table. I'd prefer an implementation that does need a
specific entry for each variant of 8[356]xx.
--
Timur Tabi
Linux kernel developer at Freescale
So we need to update this table every time there's a new SOC? All 83xx, 85xx,
and 86xx SOCs use the same table. I'd prefer an implementation that does need a
specific entry for each variant of 8[356]xx.
We could add a property "source-clock-divider = <2/3>" if it's needed!?
Wolfgang.
From: Grant Likely <hidden> Date: 2008-07-31 18:07:56
On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger [off-list ref] wrote:
We could add a property "source-clock-divider = <2/3>" if it's needed!?
fsl,source-clock-divider
But, yes, this is a good solution (assuming that it is a board or SoC
characteristic, and not just a choice that the driver has the option
of making on it's own).
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
So we need to update this table every time there's a new SOC? All 83xx, 85xx,
and 86xx SOCs use the same table. I'd prefer an implementation that does need a
specific entry for each variant of 8[356]xx.
This is a solved problem. The device tree simple claims compatibility
with an older part that has the identical register-level interface.
For example; an mpc8540 based board would claim:
compatible = "fsl,mpc8540-i2c"
MPC8541/60/55,MPC8610 would all then claim:
compatible = "fsl,<exact-soc>-i2c", "fsl,mpc8540-i2c"
Cheers,
g.
From: Timur Tabi <hidden> Date: 2008-07-31 18:10:30
Grant Likely wrote:
On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger [off-list ref] wrote:
quoted
We could add a property "source-clock-divider = <2/3>" if it's needed!?
fsl,source-clock-divider
But, yes, this is a good solution (assuming that it is a board or SoC
characteristic, and not just a choice that the driver has the option
of making on it's own).
I was going to suggest the actual clock frequency of the I2C device. It would
be value of gd->i2c1_clk or gd->i2c2_clk from U-Boot. The actual divider value
is meaningless.
--
Timur Tabi
Linux kernel developer at Freescale
From: Timur Tabi <hidden> Date: 2008-07-31 18:13:10
Grant Likely wrote:
This is a solved problem. The device tree simple claims compatibility
with an older part that has the identical register-level interface.
That would assume that the clock frequency is the only thing that decides
compatibility, which may technically be true now, but I don't think it's a good
idea.
I don't understand what's wrong with simply specifying the actual clock
frequency that the device uses? It varies from SOC to SOC, but U-Boot
calculates today already:
#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
gd->i2c1_clk = sys_info.freqSystemBus;
#elif defined(CONFIG_MPC8544)
/*
* 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;
We need this ugliness in U-Boot. Let's take advantage of this and do something
clean and elegant in the device tree.
--
Timur Tabi
Linux kernel developer at Freescale
From: Grant Likely <hidden> Date: 2008-07-31 18:21:42
On Thu, Jul 31, 2008 at 01:10:30PM -0500, Timur Tabi wrote:
Grant Likely wrote:
quoted
On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger [off-list ref] wrote:
quoted
We could add a property "source-clock-divider = <2/3>" if it's needed!?
fsl,source-clock-divider
But, yes, this is a good solution (assuming that it is a board or SoC
characteristic, and not just a choice that the driver has the option
of making on it's own).
I was going to suggest the actual clock frequency of the I2C device. It would
be value of gd->i2c1_clk or gd->i2c2_clk from U-Boot. The actual divider value
is meaningless.
I'm cool with that too, as long as it gets documented
g.
From: Grant Likely <hidden> Date: 2008-07-31 18:28:10
On Thu, Jul 31, 2008 at 01:13:10PM -0500, Timur Tabi wrote:
Grant Likely wrote:
quoted
This is a solved problem. The device tree simple claims compatibility
with an older part that has the identical register-level interface.
That would assume that the clock frequency is the only thing that decides
compatibility, which may technically be true now, but I don't think it's a good
idea.
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that. I suggested encoding
the clock divider directly in compatible (implicit in the SoC version),
but it doesn't have to be that way. If clock freq is obtained from
another property or some other method then that is okay too.
What is important is that if compatibility is claimed, then it must
really be compatible! If some new part appears in the future that
breaks our assumptions, then we'll need to rework the driver anyway and
the new part will *not* claim compatibility with the old.
I don't understand what's wrong with simply specifying the actual clock
frequency that the device uses? It varies from SOC to SOC, but U-Boot
calculates today already:
There is nothing wrong with it (as long as we agree and it gets
documented). I certainly don't have a problem with doing it this way.
g.
From: Timur Tabi <hidden> Date: 2008-07-31 18:35:51
Grant Likely wrote:
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
That answer is dependent on the actual clock input to the device, which is
external to the device. I wouldn't call the input frequency a property of the
I2C device.
I suggested encoding
the clock divider directly in compatible (implicit in the SoC version),
but it doesn't have to be that way. If clock freq is obtained from
another property or some other method then that is okay too.
I think we agree on that.
There is nothing wrong with it (as long as we agree and it gets
documented). I certainly don't have a problem with doing it this way.
I propose the property "clock-frequency", like this:
i2c@3000 {
#address-cells = <1>;
#size-cells = <0>;
cell-index = <0>;
compatible = "fsl-i2c";
reg = <0x3000 0x100>;
interrupts = <14 0x8>;
interrupt-parent = <&ipic>;
dfsrr;
clock-frequency = <0xblablabla>; <-- added by U-Boot
};
Note that the dfsrr property already differentiates between 8xxx and 52xx, so
maybe we don't need any other device tree changes.
--
Timur Tabi
Linux kernel developer at Freescale
From: Jon Smirl <hidden> Date: 2008-07-31 18:57:25
On 7/31/08, Timur Tabi [off-list ref] wrote:
Grant Likely wrote:
> No it doesn't, it depends on the register interface to decide
> compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
That answer is dependent on the actual clock input to the device, which is
external to the device. I wouldn't call the input frequency a property of the
I2C device.
> I suggested encoding
> the clock divider directly in compatible (implicit in the SoC version),
> but it doesn't have to be that way. If clock freq is obtained from
> another property or some other method then that is okay too.
I think we agree on that.
> There is nothing wrong with it (as long as we agree and it gets
> documented). I certainly don't have a problem with doing it this way.
I propose the property "clock-frequency", like this:
i2c@3000 {
#address-cells = <1>;
#size-cells = <0>;
cell-index = <0>;
compatible = "fsl-i2c";
reg = <0x3000 0x100>;
interrupts = <14 0x8>;
interrupt-parent = <&ipic>;
dfsrr;
The existence of the dfsrr and fsl,mpc5200-i2c attributes imply to me
that the compatible strings were not done correctly. If these devices
really were compatible we wouldn't need extra attributes to tell them
apart.
So I'm with Grant on adding compatible strings sufficient to remove
the need for dfsrr and fsl,mpc5200-i2c.
Something like...
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = "fsl,mpc5200b-i2c", .data = fsl_i2c_mpc5200, },
{.compatible = "fsl,mpc5200-i2c", .data = fsl_i2c_mpc5200, },
{.compatible = "fsl,mpc8xxx-i2c", .data = fsl_i2c_dfsrr, },
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.
I'd like to move towards a more generic uboot that gets the processor
minimally running and then use the device tree to customize as many
things as possible. But that's just my opinion.
--
Jon Smirl
jonsmirl@gmail.com
From: Timur Tabi <hidden> Date: 2008-07-31 19:01:42
Jon Smirl wrote:
The existence of the dfsrr and fsl,mpc5200-i2c attributes imply to me
that the compatible strings were not done correctly. If these devices
really were compatible we wouldn't need extra attributes to tell them
apart.
I agree with that.
So I'm with Grant on adding compatible strings sufficient to remove
the need for dfsrr and fsl,mpc5200-i2c.
Let's just make sure we don't break backwards compatibility.
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.
I don't want to differentiate between 52xx and 8xxx any more than we have to.
If 8xxx has the clock frequency in the device tree, then 52xx should have it
there, too.
For backwards compatibility purposes, we can have the driver provide a
hard-coded value of some kind if the property does not exist.
I'd like to move towards a more generic uboot that gets the processor
minimally running and then use the device tree to customize as many
things as possible. But that's just my opinion.
U-Boot needs to configure the I2C bus speed because U-Boot uses the I2C. We
should capitalize on that by taking the information that U-Boot has calculated
and re-use it.
--
Timur Tabi
Linux kernel developer at Freescale
From: Scott Wood <hidden> Date: 2008-07-31 19:01:46
Timur Tabi wrote:
Grant Likely wrote:
quoted
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
If it affects the values you need to write to the registers to achieve a
given result, how is it not a difference in the register interface?
A clock-frequency property is OK, and is in line with what we do in
other types of nodes. However, in the long run it might be nice to
introduce some sort of clock binding where, for example, the i2c node
can point to a clock elsewhere in the device tree as an input clock.
That way, less knowledge is required by the firmware to poke values all
over the place, and it also allows one to describe situations where the
frequency of the input clock can change (such as in low-power modes).
-Scott
From: Timur Tabi <hidden> Date: 2008-07-31 19:08:11
Scott Wood wrote:
Timur Tabi wrote:
quoted
Grant Likely wrote:
quoted
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
If it affects the values you need to write to the registers to achieve a
given result, how is it not a difference in the register interface?
I think we're splitting hairs, here. The code for actually programming the
registers is the same, regardless of the input frequency. It's just that the
input frequency is a value needed to calculate the right value. But like I
said, I don't think this distinction is that relevant.
A clock-frequency property is OK, and is in line with what we do in
other types of nodes. However, in the long run it might be nice to
introduce some sort of clock binding where, for example, the i2c node
can point to a clock elsewhere in the device tree as an input clock.
The only problem with that is that the actual input clock to the I2C device is
not the same as any other device. It's a unique clock. Look at the code I had
to write to figure out this clock just on 85xx:
/*
* The base clock for I2C depends on the actual SOC. Unfortunately,
* there is no pattern that can be used to determine the frequency, so
* the only choice is to look up the actual SOC number and use the value
* for that SOC. This information is taken from application note
* AN2919.
*/
#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
gd->i2c1_clk = sys_info.freqSystemBus;
#elif defined(CONFIG_MPC8544)
/*
* 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
This is just for 85xx, and it only applies to the I2C devices.
That way, less knowledge is required by the firmware to poke values all
over the place, and it also allows one to describe situations where the
frequency of the input clock can change (such as in low-power modes).
I don't think it's possible to do what you want to do. I2C clocking is
completely screwed up, and that's just the way the hardware was designed.
--
Timur Tabi
Linux kernel developer at Freescale
From: Jon Smirl <hidden> Date: 2008-07-31 19:11:55
On 7/31/08, Scott Wood [off-list ref] wrote:
Timur Tabi wrote:
quoted
Grant Likely wrote:
quoted
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what
specific
quoted
values to put in the FDR and DFSR registers to get a desired I2C bus
speed.
quoted
If it affects the values you need to write to the registers to achieve a
given result, how is it not a difference in the register interface?
quoted
I propose the property "clock-frequency", like this:
i2c@3000 {
#address-cells = <1>;
#size-cells = <0>;
cell-index = <0>;
compatible = "fsl-i2c";
reg = <0x3000 0x100>;
interrupts = <14 0x8>;
interrupt-parent = <&ipic>;
dfsrr;
clock-frequency = <0xblablabla>; <-- added by
U-Boot
quoted
};
A clock-frequency property is OK, and is in line with what we do in other
types of nodes. However, in the long run it might be nice to introduce some
sort of clock binding where, for example, the i2c node can point to a clock
elsewhere in the device tree as an input clock.
That way, less knowledge is required by the firmware to poke values all
over the place, and it also allows one to describe situations where the
frequency of the input clock can change (such as in low-power modes).
PowerPC,5200@0 {
timebase-frequency = <0>; /* From Bootloader */
bus-frequency = <0>; /* From Bootloader */
clock-frequency = <0>; /* From Bootloader */
};
The mpc5200 code already has mpc52xx_find_ipb_freq() to get it.
I should grep before suggesting things.
--
Jon Smirl
jonsmirl@gmail.com
From: Grant Likely <hidden> Date: 2008-07-31 19:14:51
On Thu, Jul 31, 2008 at 01:35:51PM -0500, Timur Tabi wrote:
Grant Likely wrote:
quoted
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
That answer is dependent on the actual clock input to the device, which is
external to the device. I wouldn't call the input frequency a property of the
I2C device.
Okay, I accept that argument. Make input frequency a property and be
done with it.
quoted
I suggested encoding
the clock divider directly in compatible (implicit in the SoC version),
but it doesn't have to be that way. If clock freq is obtained from
another property or some other method then that is okay too.
I think we agree on that.
indeed.
quoted
There is nothing wrong with it (as long as we agree and it gets
documented). I certainly don't have a problem with doing it this way.
Note that the dfsrr property already differentiates between 8xxx and 52xx, so
maybe we don't need any other device tree changes.
The whole dfsrr property is a really bad idea, and it just replaces the
equally bad idea of an "mpc5200-clocking" property which is currently in
use. This bit should definitely be determined via compatible.
g.
From: Scott Wood <hidden> Date: 2008-07-31 19:15:53
Timur Tabi wrote:
Scott Wood wrote:
quoted
A clock-frequency property is OK, and is in line with what we do in
other types of nodes. However, in the long run it might be nice to
introduce some sort of clock binding where, for example, the i2c node
can point to a clock elsewhere in the device tree as an input clock.
The only problem with that is that the actual input clock to the I2C device is
not the same as any other device. It's a unique clock. Look at the code I had
to write to figure out this clock just on 85xx:
IIRC, only the divider is unique, and the divider that is applied to the
input clock can be specified in the i2c node (either implicitly in
compatible, or explicitly via a property).
-Scott
From: Timur Tabi <hidden> Date: 2008-07-31 19:19:51
Scott Wood wrote:
Timur Tabi wrote:
quoted
Scott Wood wrote:
quoted
A clock-frequency property is OK, and is in line with what we do in
other types of nodes. However, in the long run it might be nice to
introduce some sort of clock binding where, for example, the i2c node
can point to a clock elsewhere in the device tree as an input clock.
The only problem with that is that the actual input clock to the I2C device is
not the same as any other device. It's a unique clock. Look at the code I had
to write to figure out this clock just on 85xx:
IIRC, only the divider is unique, and the divider that is applied to the
input clock can be specified in the i2c node (either implicitly in
compatible, or explicitly via a property).
True, but I'd rather we have a real clock-frequency property that contains the
calculated I2C input frequency, than a divider. It's more consistent with other
properties, and it hides the complicated nature of I2C clocking.
--
Timur Tabi
Linux kernel developer at Freescale
From: Scott Wood <hidden> Date: 2008-07-31 19:21:19
Timur Tabi wrote:
True, but I'd rather we have a real clock-frequency property that contains the
calculated I2C input frequency, than a divider. It's more consistent with other
properties,
Right, I wasn't saying i2c should be different from serial, pci, etc.
It was more of a side comment about the way we specify clocks in general.
-Scott
From: Timur Tabi <hidden> Date: 2008-07-31 19:22:29
Scott Wood wrote:
Timur Tabi wrote:
quoted
True, but I'd rather we have a real clock-frequency property that contains the
calculated I2C input frequency, than a divider. It's more consistent with other
properties,
Right, I wasn't saying i2c should be different from serial, pci, etc.
It was more of a side comment about the way we specify clocks in general.
I guess it's agreed then. If no one else gets around to it, I'll submit the
U-Boot patch for updating the device tree.
--
Timur Tabi
Linux kernel developer at Freescale
From: Timur Tabi <hidden> Date: 2008-07-31 19:24:41
Wolfgang Grandegger wrote:
No, the source clock is not identical for all 8[356]xx. Some use half or
even a third of the SOC clock frequency.
The platform clock divided by 2 or 3 *is* the source clock to the I2C. That's
what I'm talking about.
Linux must determine the real
source clock frequency somehow. We may introduce the SOC property
"i2c-clock-frequency", which could be fixed up by U-Boot or a pre-loader
(in case U-Boot is not used). Like for other frequency properties as well.
That's what I'm proposing, but drop the "i2c-".
--
Timur Tabi
Linux kernel developer at Freescale
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 19:24:55
Timur Tabi wrote:
Grant Likely wrote:
quoted
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
That answer is dependent on the actual clock input to the device, which is
external to the device. I wouldn't call the input frequency a property of the
I2C device.
No, the source clock is not identical for all 8[356]xx. Some use half or
even a third of the SOC clock frequency. Linux must determine the real
source clock frequency somehow. We may introduce the SOC property
"i2c-clock-frequency", which could be fixed up by U-Boot or a pre-loader
(in case U-Boot is not used). Like for other frequency properties as well.
quoted
I suggested encoding
the clock divider directly in compatible (implicit in the SoC version),
but it doesn't have to be that way. If clock freq is obtained from
another property or some other method then that is okay too.
I think we agree on that.
quoted
There is nothing wrong with it (as long as we agree and it gets
documented). I certainly don't have a problem with doing it this way.
I think we already agreed to use the property "clock-frequency" for the
real I2C clock frequency. If it is not provided, the old settings, e.g.
from U-Boot, will be kept.
Note that the dfsrr property already differentiates between 8xxx and 52xx, so
maybe we don't need any other device tree changes.
It should be done via the compatible property.
Wolfgang.
From: Grant Likely <hidden> Date: 2008-07-31 19:25:33
On Thu, Jul 31, 2008 at 02:57:25PM -0400, Jon Smirl wrote:
On 7/31/08, Timur Tabi [off-list ref] wrote:
quoted
Grant Likely wrote:
I propose the property "clock-frequency", like this:
i2c@3000 {
#address-cells = <1>;
#size-cells = <0>;
cell-index = <0>;
compatible = "fsl-i2c";
reg = <0x3000 0x100>;
interrupts = <14 0x8>;
interrupt-parent = <&ipic>;
dfsrr;
The existence of the dfsrr and fsl,mpc5200-i2c attributes imply to me
that the compatible strings were not done correctly. If these devices
really were compatible we wouldn't need extra attributes to tell them
apart.
Yes, exactly right.
So I'm with Grant on adding compatible strings sufficient to remove
the need for dfsrr and fsl,mpc5200-i2c.
Something like...
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = "fsl,mpc5200b-i2c", .data = fsl_i2c_mpc5200, },
{.compatible = "fsl,mpc5200-i2c", .data = fsl_i2c_mpc5200, },
{.compatible = "fsl,mpc8xxx-i2c", .data = fsl_i2c_dfsrr, },
Yes, but I don't agree with the last entry in this list.
"fsl,mpc8xxx-i2c" is not a good value. Use an actual part number
instead and have newer devices claim compatibility with the old.
The problem with 8xxx is that you don't know if/when another 8xxx
will arrive on the scene which does not conform to already made
assumptions. If you specify an exact SoC part, then the problem goes
away without any downside.
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.
In general, I don't like adding additional global or machine vars to
Linux. I just don't think it in necessary and it can quickly get out of
control. Instead, For 5200 I've exported a mpc52xx specific function
that returns the clock frequency. The function can be adapted over time
to handle new cases on the 52xx platform and it get compiled out if 5200
is not in use.
As an alternative, clock-frequency could be made an optional property.
If it isn't specified, then get the bus-frequency property from the
parent node instead.
g.
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 19:54:48
Timur Tabi wrote:
Wolfgang Grandegger wrote:
quoted
No, the source clock is not identical for all 8[356]xx. Some use half or
even a third of the SOC clock frequency.
The platform clock divided by 2 or 3 *is* the source clock to the I2C. That's
what I'm talking about.
quoted
Linux must determine the real
source clock frequency somehow. We may introduce the SOC property
"i2c-clock-frequency", which could be fixed up by U-Boot or a pre-loader
(in case U-Boot is not used). Like for other frequency properties as well.
That's what I'm proposing, but drop the "i2c-".
But clock-frequency, aka bus-frequency, is already used by
fsl_get_sys_freq():
http://lxr.linux.no/linux+v2.6.26/arch/powerpc/sysdev/fsl_soc.c#L80
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:
soc8541@e0000000 {
#address-cells = <1>;
#size-cells = <1>;
device_type = "soc";
ranges = <0x0 0xe0000000 0x100000>;
reg = <0xe0000000 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().
Wolfgang.
So? clock-frequency is a per-node property. I use it in the codec node on the
8610 (mpc8610_hpcd.dts). It does not mean "platform clock frequency".
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
This is just more complicated than it needs to be. Why should the I2C driver
fetch the platform clock and the divider from the parent node, and then do
additional math, when it could just get the value it needs right from the node
it's probing?
Besides, U-Boot does not currently store the divider value. Look at the code
I've posted twice already - it stores the frequency in i2c1_clk. So now I would
need to create another variable in the gd_t to store the divider? No thanks.
--
Timur Tabi
Linux kernel developer at Freescale
From: Grant Likely <hidden> Date: 2008-07-31 19:59:11
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:
soc8541@e0000000 {
#address-cells = <1>;
#size-cells = <1>;
device_type = "soc";
ranges = <0x0 0xe0000000 0x100000>;
reg = <0xe0000000 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().
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.
g.
From: Timur Tabi <hidden> Date: 2008-07-31 20:00:55
Grant Likely wrote:
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.
His proposal would make the implementation less simple, not more simple.
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.
It isn't. I don't have the Verilog for the I2C device, but I'm pretty sure the
divider is a very simple circuit that's located just outside the I2C circuitry,
but it still unique to the I2C device. There is no "I2C clock" running through
the SOC.
--
Timur Tabi
Linux kernel developer at Freescale
So? clock-frequency is a per-node property. I use it in the codec node on the
8610 (mpc8610_hpcd.dts). It does not mean "platform clock frequency".
quoted
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
This is just more complicated than it needs to be. Why should the I2C driver
fetch the platform clock and the divider from the parent node, and then do
additional math, when it could just get the value it needs right from the node
it's probing?
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?
Besides, U-Boot does not currently store the divider value. Look at the code
I've posted twice already - it stores the frequency in i2c1_clk. So now I would
need to create another variable in the gd_t to store the divider? No thanks.
OK, that's an argument but it's biased by U-Boot.
Wolfgang.
From: Timur Tabi <hidden> Date: 2008-07-31 20:19:16
Wolfgang Grandegger wrote:
I'm a bit confused. The frequency of the I2C source clock and the real
I2C clock frequency are two different things.
There are two frequencies:
1) The frequency of the input clock to the I2C device, after it has gone through
a divider. This is what I call the "I2C clock frequency" and what I think
belongs in the clock-frequency property. This is usually the platform clock
divided by 1, 2, or 3.
2) The speed of the I2C bus, as seen by devices on that bus. This is usually
400KHz.
The first one is common
for all I2C devices, the second can be different. What properties would
you like to use for defining both?
The platform clock has no value to the I2C hardware, so I don't care anything
about it.
quoted
Besides, U-Boot does not currently store the divider value. Look at the code
I've posted twice already - it stores the frequency in i2c1_clk. So now I would
need to create another variable in the gd_t to store the divider? No thanks.
OK, that's an argument but it's biased by U-Boot.
As long as a method that is favorable to U-Boot does not put any undo hardship
on non-U-Boot methods, I would say that it is the preferred method.
--
Timur Tabi
Linux kernel developer at Freescale
From: Timur Tabi <hidden> Date: 2008-07-31 20:19:58
Wolfgang Grandegger wrote:
Is it not exactly that? For me it's not a per I2C device property, at least.
Of course it's a per-I2C device property. The input frequency to the I2C device
is only seen by the I2C device, and no other device.
--
Timur Tabi
Linux kernel developer at Freescale
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 20:20:50
Grant Likely wrote:
On Thu, Jul 31, 2008 at 09:54:48PM +0200, Wolfgang Grandegger wrote:
quoted
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:
soc8541@e0000000 {
#address-cells = <1>;
#size-cells = <1>;
device_type = "soc";
ranges = <0x0 0xe0000000 0x100000>;
reg = <0xe0000000 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().
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.
Is it not exactly that? For me it's not a per I2C device property, at least.
Wolfgang.
From: Timur Tabi <hidden> Date: 2008-07-31 20:28:05
Wolfgang Grandegger wrote:
quoted
2) The speed of the I2C bus, as seen by devices on that bus. This is usually
400KHz.
Which should be defined with the property "current-speed", right?
I would use something like "bus-speed", but yes. The word "current" shouldn't
be in a property string.
--
Timur Tabi
Linux kernel developer at Freescale
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 20:28:55
Timur Tabi wrote:
Wolfgang Grandegger wrote:
quoted
I'm a bit confused. The frequency of the I2C source clock and the real
I2C clock frequency are two different things.
There are two frequencies:
1) The frequency of the input clock to the I2C device, after it has gone through
a divider. This is what I call the "I2C clock frequency" and what I think
belongs in the clock-frequency property. This is usually the platform clock
divided by 1, 2, or 3.
OK.
2) The speed of the I2C bus, as seen by devices on that bus. This is usually
400KHz.
Which should be defined with the property "current-speed", right?
Wolfgang.
From: Grant Likely <hidden> Date: 2008-07-31 20:30:34
On Thu, Jul 31, 2008 at 2:19 PM, Timur Tabi [off-list ref] wrote:
Wolfgang Grandegger wrote:
quoted
I'm a bit confused. The frequency of the I2C source clock and the real
I2C clock frequency are two different things.
There are two frequencies:
1) The frequency of the input clock to the I2C device, after it has gone through
a divider. This is what I call the "I2C clock frequency" and what I think
belongs in the clock-frequency property. This is usually the platform clock
divided by 1, 2, or 3.
2) The speed of the I2C bus, as seen by devices on that bus. This is usually
400KHz.
analogous example: serial nodes use two properties; "clock-frequency"
and "current-speed", one for clock frequency routed into the device
and one for the current baud rate. It is completely relevant to put
the effective clock frequency into the i2c device node.
g.
So? clock-frequency is a per-node property. I use it in the codec node
on the
quoted
8610 (mpc8610_hpcd.dts). It does not mean "platform clock frequency".
quoted
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
quoted
quoted
This is just more complicated than it needs to be. Why should the I2C
driver
quoted
fetch the platform clock and the divider from the parent node, and then do
additional math, when it could just get the value it needs right from the
node
quoted
it's probing?
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?
For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get
the source clock for the i2c devices. Each i2c node in the device tree
can then specify "clock-frequency = 400000;" or let it default. You
have the input clock and the desired clock, do some math and you can
set FDR.
For the 8xxx chips there is no simple solution for obtain the input
clock for the i2c section. Maybe create something like i2c-frequency
and set it from uboot. The make another accessor function like
mpc8xxx_find_i2c_freq().
PowerPC,8xxx@0 {
timebase-frequency = <0>; /* From Bootloader */
bus-frequency = <0>; /* From Bootloader */
clock-frequency = <0>; /* From Bootloader */
i2c-frequency = <0>; /* From Bootloader */
};
Instead of creating i2c-frequency in the device tree, you could put
this piece of code in the the mpc8xxx platform driver and use it to
implement mpc8xxx_find_i2c_freq(). Read the PORDEVSR2_SEC_CFG bit back
from whatever uboot set it too.
#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
gd->i2c1_clk = sys_info.freqSystemBus;
#elif defined(CONFIG_MPC8544)
/*
* 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;
--
Jon Smirl
jonsmirl@gmail.com
So? clock-frequency is a per-node property. I use it in the codec node
on the
quoted
8610 (mpc8610_hpcd.dts). It does not mean "platform clock frequency".
quoted
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
quoted
quoted
This is just more complicated than it needs to be. Why should the I2C
driver
quoted
fetch the platform clock and the divider from the parent node, and then do
additional math, when it could just get the value it needs right from the
node
quoted
it's probing?
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? a
shared register setting? or a register setting within the i2c device?
(My answer depends on the layout)
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
From: Timur Tabi <hidden> Date: 2008-07-31 20:35:36
Jon Smirl wrote:
For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get
the source clock for the i2c devices. Each i2c node in the device tree
can then specify "clock-frequency = 400000;" or let it default. You
400KHz is the *speed* of the I2C bus, so let's be sure to use "speed" in the
property name. Reserve the word "bus" for the input clock to the I2C device.
#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
gd->i2c1_clk = sys_info.freqSystemBus;
#elif defined(CONFIG_MPC8544)
/*
* 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;
I think the whole point is to eliminate duplicating this code in the Linux
driver. It's hideously ugly, so it should be limited as much as possible.
--
Timur Tabi
Linux kernel developer at Freescale
From: Timur Tabi <hidden> Date: 2008-07-31 20:37:07
Grant Likely wrote:
How is the divider controlled? Is it a fixed property of the SoC?
Yes. The divider is either 1, 2, or 3, and the only way to know which one it is
is to look up the specific SOC model number. And depending on the SOC model,
there may also be a register that needs to be looked up.
a
shared register setting? or a register setting within the i2c device?
The I2C device itself has no idea what the divider is. It only sees the result
of the divider.
--
Timur Tabi
Linux kernel developer at Freescale
From: Jon Smirl <hidden> Date: 2008-07-31 20:43:40
On 7/31/08, Timur Tabi [off-list ref] wrote:
Jon Smirl wrote:
> For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get
> the source clock for the i2c devices. Each i2c node in the device tree
> can then specify "clock-frequency = 400000;" or let it default. You
400KHz is the *speed* of the I2C bus, so let's be sure to use "speed" in the
property name. Reserve the word "bus" for the input clock to the I2C device.
> #if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
> defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
> gd->i2c1_clk = sys_info.freqSystemBus;
> #elif defined(CONFIG_MPC8544)
> /*
> * 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;
I think the whole point is to eliminate duplicating this code in the Linux
driver. It's hideously ugly, so it should be limited as much as possible.
It wouldn't go into the i2c driver, it would go into the mpc8xxx
platform driver. Why is it bad to put it into the mpc8xxx platform
driver? It is an accurate description of the mpc8xxx platform isn't
it?
--
Jon Smirl
jonsmirl@gmail.com
From: Timur Tabi <hidden> Date: 2008-07-31 20:44:54
Jon Smirl wrote:
It wouldn't go into the i2c driver, it would go into the mpc8xxx
platform driver. Why is it bad to put it into the mpc8xxx platform
driver? It is an accurate description of the mpc8xxx platform isn't
it?
There's no need to put that code in the platform driver because U-Boot will put
the final result in the device tree! That way, we won't need to update the
platform driver *and* U-Boot for any new SOC.
--
Timur Tabi
Linux kernel developer at Freescale
From: Grant Likely <hidden> Date: 2008-07-31 20:48:38
On Thu, Jul 31, 2008 at 03:37:07PM -0500, Timur Tabi wrote:
Grant Likely wrote:
quoted
How is the divider controlled? Is it a fixed property of the SoC?
Yes. The divider is either 1, 2, or 3, and the only way to know which one
it is is to look up the specific SOC model number. And depending on the
SOC model, there may also be a register that needs to be looked up.
quoted
a
shared register setting? or a register setting within the i2c device?
The I2C device itself has no idea what the divider is. It only sees the
result of the divider.
Then that absolutely suggests to me that either the final clock or the
divider should be encoded in the i2c node; not in the soc node.
g.
From: Jon Smirl <hidden> Date: 2008-07-31 20:55:20
On 7/31/08, Grant Likely [off-list ref] wrote:
On Thu, Jul 31, 2008 at 03:37:07PM -0500, Timur Tabi wrote:
> Grant Likely wrote:
>
> > How is the divider controlled? Is it a fixed property of the SoC?
>
> Yes. The divider is either 1, 2, or 3, and the only way to know which one
> it is is to look up the specific SOC model number. And depending on the
> SOC model, there may also be a register that needs to be looked up.
>
> > a
> > shared register setting? or a register setting within the i2c device?
>
> The I2C device itself has no idea what the divider is. It only sees the
> result of the divider.
Then that absolutely suggests to me that either the final clock or the
divider should be encoded in the i2c node; not in the soc node.
Isn't there a single global divider that generates all the i2c source
clocks? You don't want to copy a global value into each i2c node.
Aren't we talking about the /2 or /3 or /1 divider that appears to be
randomly implemented on various members of the mpc8xxx family?
From: Scott Wood <hidden> Date: 2008-07-31 20:56:39
Jon Smirl wrote:
Isn't there a single global divider that generates all the i2c source
clocks? You don't want to copy a global value into each i2c node.
Why not? The "compatible" is pretty much always going to be the same
for the i2c node, but we copy that, as well.
Likewise for the clock-frequency of serial nodes.
-Scott
From: Timur Tabi <hidden> Date: 2008-07-31 20:56:53
Jon Smirl wrote:
Isn't there a single global divider that generates all the i2c source
clocks? You don't want to copy a global value into each i2c node.
Why not? There are only two I2C devices, and it's theoretically possible for
them to have different input clock frequencies. Keeping it in the I2C node
allows the I2C driver to reference a property directly in the node that its probing.
Aren't we talking about the /2 or /3 or /1 divider that appears to be
randomly implemented on various members of the mpc8xxx family?
Yes.
--
Timur Tabi
Linux kernel developer at Freescale
From: Jon Smirl <hidden> Date: 2008-07-31 21:03:07
On 7/31/08, Timur Tabi [off-list ref] wrote:
Jon Smirl wrote:
> Isn't there a single global divider that generates all the i2c source
> clocks? You don't want to copy a global value into each i2c node.
Why not? There are only two I2C devices, and it's theoretically possible for
them to have different input clock frequencies. Keeping it in the I2C node
allows the I2C driver to reference a property directly in the node that its probing.
But that's the same as saying we should copy the system clock
frequency into all of the PSC nodes because we might implement
hardware where they aren't all clocked off from the same input clock
source.
> Aren't we talking about the /2 or /3 or /1 divider that appears to be
> randomly implemented on various members of the mpc8xxx family?
I don't this these dividers or clocks need to be exposed at all if
you'd just put that ugly code snippet into your platform driver.
Yes.
--
Timur Tabi
Linux kernel developer at Freescale
From: Timur Tabi <hidden> Date: 2008-07-31 21:10:10
Jon Smirl wrote:
But that's the same as saying we should copy the system clock
frequency into all of the PSC nodes because we might implement
hardware where they aren't all clocked off from the same input clock
source.
The I2C clock is only visible to the I2C devices. The system clock is seen by
many devices. There's the difference.
quoted
> Aren't we talking about the /2 or /3 or /1 divider that appears to be
> randomly implemented on various members of the mpc8xxx family?
I don't this these dividers or clocks need to be exposed at all if
you'd just put that ugly code snippet into your platform driver.
That's why I don't think the divider belongs in the device tree. Just put the
actual resulting clock frequency in the device tree.
Besides, putting that snippet in the platform driver *is* exposing it.
--
Timur Tabi
Linux kernel developer at Freescale
From: Wolfgang Grandegger <hidden> Date: 2008-07-31 21:14:40
Jon Smirl wrote:
On 7/31/08, Timur Tabi [off-list ref] wrote:
quoted
Jon Smirl wrote:
> Isn't there a single global divider that generates all the i2c source
> clocks? You don't want to copy a global value into each i2c node.
Why not? There are only two I2C devices, and it's theoretically possible for
them to have different input clock frequencies. Keeping it in the I2C node
allows the I2C driver to reference a property directly in the node that its probing.
But that's the same as saying we should copy the system clock
frequency into all of the PSC nodes because we might implement
hardware where they aren't all clocked off from the same input clock
source.
quoted
> Aren't we talking about the /2 or /3 or /1 divider that appears to be
> randomly implemented on various members of the mpc8xxx family?
I don't this these dividers or clocks need to be exposed at all if
you'd just put that ugly code snippet into your platform driver.
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.
Wolfgang.
From: Timur Tabi <hidden> Date: 2008-07-31 21:17:24
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.
--
Timur Tabi
Linux kernel developer at Freescale
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.
quoted
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.
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>;
};
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.
On Thu, Jul 31, 2008 at 09:54:48PM +0200, Wolfgang Grandegger wrote:
quoted
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:
soc8541@e0000000 {
#address-cells = <1>;
#size-cells = <1>;
device_type = "soc";
ranges = <0x0 0xe0000000 0x100000>;
reg = <0xe0000000 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.
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.