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

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

[Wolfgang Grandegger]

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 <wg@grandegger.com>
---
 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
===================================================================
--- linux-2.6-galak.orig/drivers/i2c/busses/i2c-mpc.c
+++ linux-2.6-galak/drivers/i2c/busses/i2c-mpc.c
@@ -56,6 +56,8 @@ struct mpc_i2c {
 	struct i2c_adapter adap;
 	int irq;
 	u32 flags;
+	u32 fdr;
+	u32 dfsrr;
 };
 
 static __inline__ void writeccr(struct mpc_i2c *i2c, u32 x)
@@ -156,13 +158,16 @@ static int i2c_wait(struct mpc_i2c *i2c,
 static void mpc_i2c_setclock(struct mpc_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);
-	} else if (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);
+	} else if (i2c->flags & FSL_I2C_DEV_CLOCK_5200) {
+		writeb(i2c->fdr, i2c->base + MPC_I2C_FDR);
+	} else {
+		writel(i2c->fdr, i2c->base + MPC_I2C_FDR);
+	}
 }
 
 static void mpc_i2c_start(struct mpc_i2c *i2c)
@@ -320,18 +325,40 @@ static int __devinit fsl_i2c_probe(struc
 {
 	int result = 0;
 	struct mpc_i2c *i2c;
+	const u32 *prop;
+	int prop_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;
+		else if (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);
Index: linux-2.6-galak/Documentation/powerpc/dts-bindings/fsl/i2c.txt
===================================================================
--- linux-2.6-galak.orig/Documentation/powerpc/dts-bindings/fsl/i2c.txt
+++ linux-2.6-galak/Documentation/powerpc/dts-bindings/fsl/i2c.txt
@@ -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 {
@@ -28,5 +30,6 @@ Example :
 		reg = <3000 18>;
 		device_type = "i2c";
 		compatible  = "fsl-i2c";
-		dfsrr;
+		fdr = <0x20>
+		dfsrr = <0x03>;
 	};

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

[Jochen Friedrich]

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

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

[Wolfgang Grandegger]

Jochen Friedrich wrote:
> 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?

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.

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

[Grant Likely]

On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Jochen Friedrich wrote:
>>
>> 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?

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.

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

[Timur Tabi]

On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely <grant.likely@secretlab.ca> 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

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

[Jon Smirl]

On 7/25/08, Timur Tabi <timur@freescale.com> wrote:
> On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely <grant.likely@secretlab.ca> 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

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

[Wolfgang Grandegger]

Timur Tabi wrote:
> On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely <grant.likely@secretlab.ca> 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 know but we still need an algorithm for MPC52xx and MPC82xx as well.

> Although I'm curious, if U-Boot already programs the speed, why does
> the driver program it again?

Maybe because it's not obvious for the driver if the registers have 
already been configured by the boot-loader.

Wolfgang.

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

[Timur Tabi]

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

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

[Grant Likely]

On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
> 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.

I agree, it should encode real frequencies, not raw register values.

g.

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

[Wolfgang Grandegger]

Grant Likely wrote:
> On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
>> 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.
> 
> 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.

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

[Jon Smirl]

On 7/31/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>
> > On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
> >
> > > 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.
> > >
> >
> > 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
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Timur Tabi]

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

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

[Trent Piepho]

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.

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

[Timur Tabi]

On Thu, Jul 31, 2008 at 6:32 PM, Trent Piepho <xyzzy@speakeasy.org> 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

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

[Scott Wood]

On Fri, Aug 01, 2008 at 08:17:25AM -0500, Timur Tabi wrote:
> On Thu, Jul 31, 2008 at 6:32 PM, Trent Piepho <xyzzy@speakeasy.org> 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

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

[Trent Piepho]

On Fri, 1 Aug 2008, Timur Tabi wrote:
> On Thu, Jul 31, 2008 at 6:32 PM, Trent Piepho <xyzzy@speakeasy.org> 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.

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

[Timur Tabi]

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

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

[Wolfgang Grandegger]

Jon Smirl wrote:
> On 7/31/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>
>>> On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
>>>
>>>> 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.
>>>>
>>> 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.

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

[Grant Likely]

On Thu, Jul 31, 2008 at 11:22 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Jon Smirl wrote:
>>
>> On 7/31/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>>
>>> Grant Likely wrote:
>>>
>>>> On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
>>>>
>>>>> 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.
>>>>>
>>>> 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.

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

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Jul 31, 2008 at 11:22 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Jon Smirl wrote:
>>> On 7/31/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>>> Grant Likely wrote:
>>>>
>>>>> On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
>>>>>
>>>>>> 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.
>>>>>>
>>>>> 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);

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.

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

[Timur Tabi]

Grant Likely wrote:

> 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, },

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

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

[Wolfgang Grandegger]

Timur Tabi wrote:
> Grant Likely wrote:
> 
>> 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, },
> 
> 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.

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

[Timur Tabi]

Wolfgang Grandegger wrote:

> We could add a property "source-clock-divider = <2/3>" if it's needed!?

How about we just get U-Boot to put the core frequency in the device tree?

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Grant Likely]

On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger <wg@grandegger.com> 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.

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

[Timur Tabi]

Grant Likely wrote:
> On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger <wg@grandegger.com> 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).

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

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

[Grant Likely]

On Thu, Jul 31, 2008 at 01:10:30PM -0500, Timur Tabi wrote:
> Grant Likely wrote:
> > On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger <wg@grandegger.com> 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).
> 
> 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.

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

[Grant Likely]

On Thu, Jul 31, 2008 at 12:54:02PM -0500, Timur Tabi wrote:
> Grant Likely wrote:
> 
> > 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, },
> 
> 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.

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

[Timur Tabi]

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

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

[Grant Likely]

On Thu, Jul 31, 2008 at 01:13:10PM -0500, Timur Tabi wrote:
> 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.

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.

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

[Timur Tabi]

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

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

[Jon Smirl]

On 7/31/08, Timur Tabi <timur@freescale.com> 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

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

[Timur Tabi]

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.

> 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, },

That seems ok.

> 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

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

[Grant Likely]

On Thu, Jul 31, 2008 at 02:57:25PM -0400, Jon Smirl wrote:
> On 7/31/08, Timur Tabi <timur@freescale.com> wrote:
> > 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.

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

[Trent Piepho]

On Thu, 31 Jul 2008, Jon Smirl wrote:
> As for the source clock, how about creating a new global like
> ppc_proc_freq called ppc_ipb_freq. The platform code can then set the
> right clock value into the variable. For mpc8xxxx get it from uboot.
> mpc5200 can easily compute it from ppc_proc_freq and checking how the
> ipb divider is set. That will move the clock problem out of the i2c
> driver.

There is a huge variation in where the I2C source clock comes from.
Sometimes it's the system bus, sometimes ethernet, sometimes SEC, etc.  If
I look at u-boot (which might not be entirely correct or complete), I see:

83xx:  5 different clock sources
85xx:  3 different clock sources
86xx:  2 different clock sources

But there's more.  Sometimes the two I2C controllers don't use the same
clock!  So even if you add 10 globals with different clocks, and then add
code to the mpc i2c driver so if can figure out which one to use given the
platform, it's still not enough because you need to know which controller
the device node is for.

IMHO, what Timur suggested of having u-boot put the source clock into the
i2c node makes the most sense.  U-boot has to figure this out, so why
duplicate the work?

Here's my idea:

	i2c@0 {
		compatible = "fsl-i2c";
		bus-frequency = <100000>;

		/* Either */
		source-clock-frequency = <0>;
		/* OR */
		source-clock = <&ccb>;
	};

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.

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

[Jon Smirl]

On 7/31/08, Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Thu, 31 Jul 2008, Jon Smirl wrote:
>  > As for the source clock, how about creating a new global like
>  > ppc_proc_freq called ppc_ipb_freq. The platform code can then set the
>  > right clock value into the variable. For mpc8xxxx get it from uboot.
>  > mpc5200 can easily compute it from ppc_proc_freq and checking how the
>  > ipb divider is set. That will move the clock problem out of the i2c
>  > driver.
>
>
> There is a huge variation in where the I2C source clock comes from.
>  Sometimes it's the system bus, sometimes ethernet, sometimes SEC, etc.  If
>  I look at u-boot (which might not be entirely correct or complete), I see:
>
>  83xx:  5 different clock sources
>  85xx:  3 different clock sources
>  86xx:  2 different clock sources
>
>  But there's more.  Sometimes the two I2C controllers don't use the same
>  clock!  So even if you add 10 globals with different clocks, and then add
>  code to the mpc i2c driver so if can figure out which one to use given the
>  platform, it's still not enough because you need to know which controller
>  the device node is for.
>
>  IMHO, what Timur suggested of having u-boot put the source clock into the
>  i2c node makes the most sense.  U-boot has to figure this out, so why
>  duplicate the work?
>
>  Here's my idea:
>
>         i2c@0 {
>                 compatible = "fsl-i2c";
>                 bus-frequency = <100000>;
>
>                 /* Either */
>                 source-clock-frequency = <0>;
>                 /* OR */
>                 source-clock = <&ccb>;
>         };

Can't we hide a lot of this on platforms where the source clock is not
messed up? For example the mpc5200 doesn't need any of this, the
needed frequency is already available in mpc52xx_find_ipb_freq().
mpc5200 doesn't need any uboot change.

Next would be normal mpc8xxx platforms where i2c is driven by a single
clock, add a uboot filled in parameter in the root node (or I think it
can be computed off of the ones uboot is already filling in). make a
mpc8xxx_find_i2c_freq() function. May not need to change device tree
and uboot.

Finally use this for those days when the tea leaves were especially
bad. Both a device tree and uboot change.

> Except the i2c clock isn't always a based on an integer divider of the CCB
>  frequency.  What's more, it's not always the same for both i2c controllers.
>  Suppose i2c #1 uses CCB times 2/3 and i2c #2 uses CCB/2, how would
>  fsl_get_i2c_freq() figure that out from bus-frequency and
>  i2c-clock-divider?

If you get the CCB frequency from uboot and know the chip model, can't
you compute these in the platform code? Then make a
mpc8xxx_find_i2c_freq(cell_index).

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
jonsmirl@gmail.com

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

[Trent Piepho]

On Thu, 31 Jul 2008, Jon Smirl wrote:
> >  Here's my idea:
> >
> >         i2c@0 {
> >                 compatible = "fsl-i2c";
> >                 bus-frequency = <100000>;
> >
> >                 /* Either */
> >                 source-clock-frequency = <0>;
> >                 /* OR */
> >                 source-clock = <&ccb>;
> >         };
>
> Can't we hide a lot of this on platforms where the source clock is not
> messed up? For example the mpc5200 doesn't need any of this, the
> needed frequency is already available in mpc52xx_find_ipb_freq().
> mpc5200 doesn't need any uboot change.
>
> Next would be normal mpc8xxx platforms where i2c is driven by a single
> clock, add a uboot filled in parameter in the root node (or I think it
> can be computed off of the ones uboot is already filling in). make a
> mpc8xxx_find_i2c_freq() function. May not need to change device tree
> and uboot.
>
> Finally use this for those days when the tea leaves were especially
> bad. Both a device tree and uboot change.

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?

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

[Jon Smirl]

On 7/31/08, Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Thu, 31 Jul 2008, Jon Smirl wrote:
>
> > >  Here's my idea:
>  > >
>  > >         i2c@0 {
>  > >                 compatible = "fsl-i2c";
>  > >                 bus-frequency = <100000>;
>  > >
>  > >                 /* Either */
>  > >                 source-clock-frequency = <0>;
>  > >                 /* OR */
>  > >                 source-clock = <&ccb>;
>  > >         };
>  >
>  > Can't we hide a lot of this on platforms where the source clock is not
>  > messed up? For example the mpc5200 doesn't need any of this, the
>  > needed frequency is already available in mpc52xx_find_ipb_freq().
>  > mpc5200 doesn't need any uboot change.
>  >
>  > Next would be normal mpc8xxx platforms where i2c is driven by a single
>  > clock, add a uboot filled in parameter in the root node (or I think it
>  > can be computed off of the ones uboot is already filling in). make a
>  > mpc8xxx_find_i2c_freq() function. May not need to change device tree
>  > and uboot.
>  >
>  > Finally use this for those days when the tea leaves were especially
>  > bad. Both a device tree and uboot change.
>
>
> 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
jonsmirl@gmail.com

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

[Timur Tabi]

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

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

[Jon Smirl]

On 8/1/08, Timur Tabi <timur@freescale.com> 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
jonsmirl@gmail.com

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

[Wolfgang Grandegger]

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.

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

[Grant Likely]

On Thu, Jul 31, 2008 at 09:19:51PM -0400, Jon Smirl wrote:
> On 7/31/08, Trent Piepho <xyzzy@speakeasy.org> wrote:
> > On Thu, 31 Jul 2008, Jon Smirl wrote:
> >  > As for the source clock, how about creating a new global like
> >  > ppc_proc_freq called ppc_ipb_freq. The platform code can then set the
> >  > right clock value into the variable. For mpc8xxxx get it from uboot.
> >  > mpc5200 can easily compute it from ppc_proc_freq and checking how the
> >  > ipb divider is set. That will move the clock problem out of the i2c
> >  > driver.
> >
> >
> > There is a huge variation in where the I2C source clock comes from.
> >  Sometimes it's the system bus, sometimes ethernet, sometimes SEC, etc.  If
> >  I look at u-boot (which might not be entirely correct or complete), I see:
> >
> >  83xx:  5 different clock sources
> >  85xx:  3 different clock sources
> >  86xx:  2 different clock sources
> >
> >  But there's more.  Sometimes the two I2C controllers don't use the same
> >  clock!  So even if you add 10 globals with different clocks, and then add
> >  code to the mpc i2c driver so if can figure out which one to use given the
> >  platform, it's still not enough because you need to know which controller
> >  the device node is for.
> >
> >  IMHO, what Timur suggested of having u-boot put the source clock into the
> >  i2c node makes the most sense.  U-boot has to figure this out, so why
> >  duplicate the work?
> >
> >  Here's my idea:
> >
> >         i2c@0 {
> >                 compatible = "fsl-i2c";
> >                 bus-frequency = <100000>;
> >
> >                 /* Either */
> >                 source-clock-frequency = <0>;
> >                 /* OR */
> >                 source-clock = <&ccb>;
> >         };
> 
> Can't we hide a lot of this on platforms where the source clock is not
> messed up? For example the mpc5200 doesn't need any of this, the
> needed frequency is already available in mpc52xx_find_ipb_freq().
> mpc5200 doesn't need any uboot change.

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.

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

[Jon Smirl]

On 7/31/08, Grant Likely <grant.likely@secretlab.ca> 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
jonsmirl@gmail.com

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

[Timur Tabi]

On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl <jonsmirl@gmail.com> 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

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

[Jon Smirl]

On 8/1/08, Timur Tabi <timur@freescale.com> wrote:
> On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl <jonsmirl@gmail.com> 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
jonsmirl@gmail.com

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

[Jon Smirl]

On 8/1/08, Timur Tabi <timur@freescale.com> wrote:
> On Thu, Jul 31, 2008 at 9:35 PM, Jon Smirl <jonsmirl@gmail.com> 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
jonsmirl@gmail.com

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

[Trent Piepho]

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 / 2;
#endif
        gd->i2c2_clk = gd->i2c1_clk;

#endif

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

[Wolfgang Grandegger]

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

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

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

   fsl_get_sys_freq() / divider.

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

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

Wolfgang.

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

[Grant Likely]

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

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.

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

[Scott Wood]

Timur Tabi 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.

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 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
> 		};

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

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

[Timur Tabi]

Scott Wood wrote:
> Timur Tabi 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.
> 
> 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

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

[Scott Wood]

Timur Tabi wrote:
> Scott Wood wrote:
>> 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

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

[Timur Tabi]

Scott Wood wrote:
> Timur Tabi wrote:
>> Scott Wood wrote:
>>> 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

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

[Scott Wood]

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

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

[Timur Tabi]

Scott Wood wrote:
> 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.

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

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

[Jon Smirl]

On 7/31/08, Scott Wood <scottwood@freescale.com> wrote:
> Timur Tabi 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.
> >
>
>  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 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
> >                };
> >
>
>  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

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

[Grant Likely]

On Thu, Jul 31, 2008 at 01:35:51PM -0500, Timur Tabi 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.

Okay, I accept that argument.  Make input frequency a property and be
done with it.

> > 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.

> > 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
> 		};

I'm okay with this.

> 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.

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

[Wolfgang Grandegger]

Timur Tabi 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.

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.

>> 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
> 		};

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.

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

[Timur Tabi]

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

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

[Wolfgang Grandegger]

Timur Tabi wrote:
> 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-".

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.

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

[Timur Tabi]

Wolfgang Grandegger wrote:

> 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

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

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

[Wolfgang Grandegger]

Timur Tabi wrote:
> Wolfgang Grandegger wrote:
> 
>> 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
> 
> 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?

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.

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

[Timur Tabi]

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.

>> 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

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

[Wolfgang Grandegger]

Timur Tabi wrote:
> 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.

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.

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

[Timur Tabi]

Wolfgang Grandegger wrote:

>> 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

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

[Grant Likely]

On Thu, Jul 31, 2008 at 2:19 PM, Timur Tabi <timur@freescale.com> wrote:
> 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.

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.

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

[Jon Smirl]

On 7/31/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Timur Tabi wrote:
>
> > Wolfgang Grandegger wrote:
> >
> >
> > > 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
> > >
> >
> > 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?
> >
>
>  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

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

[Grant Likely]

On Thu, Jul 31, 2008 at 2:32 PM, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/31/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Timur Tabi wrote:
>>
>> > Wolfgang Grandegger wrote:
>> >
>> >
>> > > 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
>> > >
>> >
>> > 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?
>> >
>>
>>  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.

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

[Timur Tabi]

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

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

[Grant Likely]

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.

g.

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

[Jon Smirl]

On 7/31/08, Grant Likely <grant.likely@secretlab.ca> 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?

>
>
>  g.
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Scott Wood]

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

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

[Timur Tabi]

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

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

[Jon Smirl]

On 7/31/08, Timur Tabi <timur@freescale.com> 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
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Timur Tabi]

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.

>>  > 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

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

[Wolfgang Grandegger]

Jon Smirl wrote:
> On 7/31/08, Timur Tabi <timur@freescale.com> 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.

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.

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

[Timur Tabi]

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

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

[Trent Piepho]

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.

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

[Trent Piepho]

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.

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

[Timur Tabi]

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

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

[Jon Smirl]

On 7/31/08, Timur Tabi <timur@freescale.com> 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

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

[Timur Tabi]

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

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

[Grant Likely]

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.

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

[Timur Tabi]

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

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

[Wolfgang Grandegger]

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:
>>
>>         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.

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

[Timur Tabi]

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

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

[Trent Piepho]

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:
> >
> >         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.

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

[Grant Likely]

On Thu, Jul 31, 2008 at 6:46 PM, Trent Piepho <xyzzy@speakeasy.org> 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.

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

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 713 bytes --]

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:   Geert.Uytterhoeven@sonycom.com
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

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

[Jon Smirl]

On 7/31/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Jon Smirl wrote:
> > 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.

>
>  Wolfgang.
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Grant Likely]

On Thu, Jul 31, 2008 at 5:51 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>>
>> On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
>>>
>>> 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.
>>
>> 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.

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

[Jon Smirl]

On 7/31/08, Grant Likely <grant.likely@secretlab.ca> 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 */
};



-- 
Jon Smirl
jonsmirl@gmail.com

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

[Grant Likely]

On Thu, Jul 31, 2008 at 11:06 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/31/08, Grant Likely <grant.likely@secretlab.ca> 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.

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.

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

[Jon Smirl]

On 7/31/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Jul 31, 2008 at 11:06 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
>  > On 7/31/08, Grant Likely <grant.likely@secretlab.ca> 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.
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Jul 31, 2008 at 5:51 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
>>>> 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.
>>> 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.

Yep, that makes sense.

Wolfgang.

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

[Wolfgang Grandegger]

Grant Likely wrote:
> On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Jochen Friedrich wrote:
>>> 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?
> 
> 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.

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.

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

[Grant Likely]

On Fri, Jul 25, 2008 at 05:34:49PM +0200, Wolfgang Grandegger wrote:
> Grant Likely wrote:
>> On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> 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.