[PATCH][RFC]Updated MPC I2C driver

This version should support MPC8540 as well as MPC8245 and MPC107. I
don't have MPC85xx hardware, so I'd like feedback from people who do.

To use it on MPC107/824x requires the OCP patch that went by on the list
on Tuesday. The support for MPC85xx is already in the kernel.

If nobody has any complaints I'm ready to send it to Greg K-H.

- Adrian Cox
Humboldt Solutions Ltd.

Hi Adrian,

	The driver works well on both an mpc8540ads and a sandpoint here.
However, I just learned yesterday that i2c 32-bit read/writes on the
85xx will not always be guaranteed to work. So there does in fact need
to be an abstraction there for 8-bit/32-bit read/writes. I wrote up a
patch that does just that and it is attached, any suggestions or
different ways to implement it I would be glad to take a go at it if
your busy. I tested the patch on our mpc8540ads and sandpoint here, and
it seems to work well. One note: the FS_I2C_32BIT needs to be set in the
flags. Let me know what you think.

Regards,
Matthew

On Thu, 2004-07-01 at 18:19, Adrian Cox wrote:

This version should support MPC8540 as well as MPC8245 and MPC107. I
don't have MPC85xx hardware, so I'd like feedback from people who do.
To use it on MPC107/824x requires the OCP patch that went by on the list
on Tuesday. The support for MPC85xx is already in the kernel.
If nobody has any complaints I'm ready to send it to Greg K-H.
- Adrian Cox
Humboldt Solutions Ltd.

--
Matthew McClintock [off-list ref]

The blocks look very similar, you might want to look at the follow up
patch I just sent that abstracts the read/writes. It might be as simple
as abstracting another read/write function for your particular platform.

Regards,
Matthew

On Thu, 2004-07-01 at 19:20, Sylvain Munaut wrote:

Adrian Cox wrote:

 > This version should support MPC8540 as well as MPC8245 and MPC107.
 > I don't have MPC85xx hardware, so I'd like feedback from people who
 > do.
 >
 > To use it on MPC107/824x requires the OCP patch that went by on the
 > list on Tuesday. The support for MPC85xx is already in the kernel.
 >
 > If nobody has any complaints I'm ready to send it to Greg K-H.


Interesting, it really looks like the MPC5200 I2C controller.
Unfortunatly :
 - On the MPC5200, all the status/controls bits are in the MSBs ( so
the consts would need to be << 24 )
 - And there is a interrupt control register I don't see in your code.
It's shared by the two I2C controllers onboard.

I'll try to see if I can make it work on the lite5200 board.

Sylvain Munaut

--
Matthew McClintock [off-list ref]


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jul 01, 2004 at 07:19:15PM +0100, Adrian Cox wrote:

[snip]

+static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
+{
+	DECLARE_WAITQUEUE(wait, current);
+        unsigned long orig_jiffies = jiffies;
+        u32 x;
+	int result = 0;
+
+	if (i2c->ocpdef->irq == OCP_IRQ_NA) {
+		while(! (readl(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
+			schedule();
+			if (orig_jiffies + timeout < jiffies) {

This doesn't look right. Please, use time_after()/time_before() here.

--
Eugene

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Adrian Cox wrote:

 > This version should support MPC8540 as well as MPC8245 and MPC107.
 > I don't have MPC85xx hardware, so I'd like feedback from people who
 > do.
 >
 > To use it on MPC107/824x requires the OCP patch that went by on the
 > list on Tuesday. The support for MPC85xx is already in the kernel.
 >
 > If nobody has any complaints I'm ready to send it to Greg K-H.


Interesting, it really looks like the MPC5200 I2C controller.
Unfortunatly :
 - On the MPC5200, all the status/controls bits are in the MSBs ( so
the consts would need to be << 24 )
 - And there is a interrupt control register I don't see in your code.
It's shared by the two I2C controllers onboard.

I'll try to see if I can make it work on the lite5200 board.

Sylvain Munaut


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, 2004-07-01 at 20:20, Sylvain Munaut wrote:

Interesting, it really looks like the MPC5200 I2C controller.
Unfortunatly :
 - On the MPC5200, all the status/controls bits are in the MSBs ( so
the consts would need to be << 24 )

Are you sure about that? The MPC107 registers are little-endian PCI
registers, accessed with readl/writel, but it looks to me that the
MPC5200 registers are big-endian.

 - And there is a interrupt control register I don't see in your code.
It's shared by the two I2C controllers onboard.

That could easily be added as a special case.

I'll try to see if I can make it work on the lite5200 board.

Please do try. It would be good if one driver covered all these chips.

I'm going to try byte reads and writes on the MPC107 as my next
experiment. I think that may avoid the need for separate access
functions.

- Adrian Cox


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Matthew McClintock wrote:

 > The blocks look very similar, you might want to look at the follow
 > up patch I just sent that abstracts the read/writes. It might be as
 > simple as abstracting another read/write function for your
 > particular platform.
 >

Yes
I'd say they are identical in fact. I kind of forgot that readl/writel
does a endianness correction. So in fact this driver should work for
the MPC5200.

My only concern is about interrupts.
MPC5200 has two I2C port:
 * One is MBAR + 0x3D00
 * Second is MBAR + 0x3D40

At MBAR + 0x3D20 there is an interrupt control register for the I2C
ports ( shared, 4bits for the first port, 4bits for the second).
 It controls for what reason an I2C interrupt should be fired. I don't
know if there is a similar register on the other, I'll look up the
datasheets.


Sylvain Munaut


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, 2004-07-01 at 15:59, Matthew McClintock wrote:

	The driver works well on both an mpc8540ads and a sandpoint here.
However, I just learned yesterday that i2c 32-bit read/writes on the
85xx will not always be guaranteed to work.

It seems that the MPC107 is happy with 8 bit accesses to the registers,
so here's another version of the patch. This also changes the time
calculations to use time_after().

This still leaves the interrupt handling register of the MPC5200
unsolved. I suggest introducing another flag for the MPC5200, and adding
a small piece of extra setup code.

- Adrian Cox
Humboldt Solutions Ltd.

Adrian Cox wrote:

It seems that the MPC107 is happy with 8 bit accesses to the registers,
so here's another version of the patch. This also changes the time
calculations to use time_after().

The MPC5200 seems happy too ;) There is a eeprom on the board and it's
detected and I can access it. If I find a soldering iron, I'll hook up
another I2C device to test other devices.

Some gotchas though :

 - The FDR computation is completly different. Here is the code I
attached the code I used in my I2C driver. Basically, it takes as
argument the desired I2C clock rate ( was a module parameter ) and the
internal bus frequency and then compute the FDR.
 - During the scan, the first bus is scanned without devices and the
error when debug is active is I2C_MAL. From the moment the eeprom is
detected, the error code become I2C_NORXACK for the address wihout
device. Included dmesg

drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x57 - 1 of 1 messages
drivers/i2c/busses/i2c-mpc.c: I2C: MAL
i2c_adapter i2c-1: found normal i2c_range entry for adapter 1, addr 0050
i2c_adapter i2c-1: master_xfer: with 1 msgs.
drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x50 - 1 of 1 messages
i2c_adapter i2c-1: master_xfer: with 1 msgs.
drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x50 - 1 of 1 messages
i2c_adapter i2c-1: client [eeprom] registered to adapter
registering 1-0050
i2c_adapter i2c-1: found normal i2c_range entry for adapter 1, addr 0051
i2c_adapter i2c-1: master_xfer: with 1 msgs.
drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x51 - 1 of 1 messages
drivers/i2c/busses/i2c-mpc.c: I2C: No RXAK
i2c_adapter i2c-1: found normal i2c_range entry for adapter 1, addr 0052
i2c_adapter i2c-1: master_xfer: with 1 msgs.

 - The interrupt bit. BTW, on what event should the interrupt be fired ?
(I did my test without any interrupts )

This still leaves the interrupt handling register of the MPC5200
unsolved. I suggest introducing another flag for the MPC5200, and adding
a small piece of extra setup code.

Yes, something like FS_I2C_IS_MPC52xx


Another remark : The register setup are done at every _start. Couldn't
they be done once for all during init ( just after the probe, call a
init_hardware functions ).



Sylvain Munaut

On Thu, 2004-07-01 at 23:32, Sylvain Munaut wrote:

 - The FDR computation is completly different. Here is the code I
attached the code I used in my I2C driver. Basically, it takes as
argument the desired I2C clock rate ( was a module parameter ) and the
internal bus frequency and then compute the FDR.

The 5200 seems to have a different FDR implementation. I could include
your calculation code if you can tell me where the value of ipb_freq
should come from.

I don't currently do a calculation on the 107, but pick a slow value for
safety. I'd need more platform specific knowledge to do the calculation,
as the 107/824x scale from the SDRAM clock, and the 85xx scales from the
CCB clock.

 - During the scan, the first bus is scanned without devices and the
error when debug is active is I2C_MAL. From the moment the eeprom is
detected, the error code become I2C_NORXACK for the address wihout
device. Included dmesg

Are you sure that it changes from MAL to NORXACK after seeing a device?
It looks to me that it returns MAL only on the bus without devices,
probably due to lack of pull-up resistors.

 - The interrupt bit. BTW, on what event should the interrupt be fired ?
(I did my test without any interrupts )

To use my driver, just set IE1 and IE2. The other flags are for a
different approach using DMA to load the TX and RX registers. That would
give a lower CPU overhead, but would also require a separate driver.

In fact, the reset value of this register is correct. The easiest fix
may be for platform code to ensure that the register has the correct
value, in case boot firmware changed it.

Another remark : The register setup are done at every _start. Couldn't
they be done once for all during init ( just after the probe, call a
init_hardware functions ).

Yes, the frequency settings could be done once on startup. I might try
that.

- Adrian Cox
Humboldt Solutions Ltd.


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Hi

Adrian Cox wrote:

The 5200 seems to have a different FDR implementation. I could include
your calculation code if you can tell me where the value of ipb_freq
should come from.

That would be nice.
The ipb_freq value is given by the boot loader or computed at run-time
if the boot loader don't give it.

#include <asm/ppcboot.h>
extern bd_t __res;
u32 ipbfreq = __res.bi_ipbfreq;

But this field will only exists when :
 - CONFIG_PPC_MPC52xx symbol is defined.
 - When the MPC52xx patch is applied to the kernel

Also, there is no DFSRR register on the 5200.

I don't currently do a calculation on the 107, but pick a slow value for
safety. I'd need more platform specific knowledge to do the calculation,
as the 107/824x scale from the SDRAM clock, and the 85xx scales from the
CCB clock.

Well yes, I guess that where the I2C modules takes it's clock from is
dependent of each platform.

Are you sure that it changes from MAL to NORXACK after seeing a device?
It looks to me that it returns MAL only on the bus without devices,
probably due to lack of pull-up resistors.

Yes, indeed, looking at the schematics indicates that it's not even I2C
and it's disabled by the port multiplexing config. I've suppressed that
entry in my ocp definition.

quoted

- The interrupt bit. BTW, on what event should the interrupt be fired ?
(I did my test without any interrupts )

To use my driver, just set IE1 and IE2. The other flags are for a
different approach using DMA to load the TX and RX registers. That would
give a lower CPU overhead, but would also require a separate driver.

Are you sure ? If I don't set the BNBE (Bus Not Busy Enable) bit, I just
get timeouts.

In fact, the reset value of this register is correct. The easiest fix
may be for platform code to ensure that the register has the correct
value, in case boot firmware changed it.

Yes sure, that's the easiest way. It's just that I'd like to avoid it.
Especially when it's content is dependent on if the user has choosed to
use irq or not.
But It's sure is a pity that the register is shared between the two I2C
... Because even with a flag, the driver should be passed the address of
this register, and what bits to use.

quoted

Another remark : The register setup are done at every _start. Couldn't
they be done once for all during init ( just after the probe, call a
init_hardware functions ).

Yes, the frequency settings could be done once on startup. I might try
that.

Thanks.
Computing the FDR each time is a little overkill ;)


Sylvain Munaut

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

It looks to me that supporting the 5200 will require a lot of small
changes.

On Fri, 2004-07-02 at 12:01, Sylvain Munaut wrote:

#include <asm/ppcboot.h>
extern bd_t __res;
u32 ipbfreq = __res.bi_ipbfreq;

But this field will only exists when :
 - CONFIG_PPC_MPC52xx symbol is defined.
 - When the MPC52xx patch is applied to the kernel

Maybe we should define two more fields in the ocp_fs_i2c_data structure:
one for base clock, and one for i2c clock. Then platform code could fill
in the clocks as necessary.

Also, there is no DFSRR register on the 5200.

I noticed that. I don't think anybody ever used anything but the default
value on the other chips.

quoted

To use my driver, just set IE1 and IE2. The other flags are for a
different approach using DMA to load the TX and RX registers. That would
give a lower CPU overhead, but would also require a separate driver.

Are you sure ? If I don't set the BNBE (Bus Not Busy Enable) bit, I just
get timeouts.

From the manual it looks as if setting BNBE might cause extra

interrupts, which the driver has no way to handle.  Could you try
enabling the interrupts, and see if this happens?

Yes sure, that's the easiest way. It's just that I'd like to avoid it.
Especially when it's content is dependent on if the user has choosed to
use irq or not.
But It's sure is a pity that the register is shared between the two I2C
... Because even with a flag, the driver should be passed the address of
this register, and what bits to use.

How about putting a function pointer for platform interrupt enabling and
disabling into the ocp_fs_i2c_data?

- Adrian Cox
Humboldt Solutions Ltd.


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Hi

Adrian Cox wrote:

On Fri, 2004-07-02 at 12:01, Sylvain Munaut wrote:

quoted

#include <asm/ppcboot.h>
extern bd_t __res;
u32 ipbfreq = __res.bi_ipbfreq;

But this field will only exists when :
- CONFIG_PPC_MPC52xx symbol is defined.
- When the MPC52xx patch is applied to the kernel

Maybe we should define two more fields in the ocp_fs_i2c_data structure:
one for base clock, and one for i2c clock. Then platform code could fill
in the clocks as necessary.

Yes, that's a good idea. It would need a flag to tell which clock
computation to take though.
Maybe instead of passing the baseclock, giving a pointer to a u32 that
contains the clock would be more appropriate. Since all board have
probably an int somewhere holding that value, you can put de definition
in the ocp_def without any further modifications.

quoted

Also, there is no DFSRR register on the 5200.

I noticed that. I don't think anybody ever used anything but the default
value on the other chips.

Yes. I've seen that the DFSRR is not as the same place. So maybe we
would need a two bits flag
like
#define FS_I2C_NO_DFSRR 0x00
#define FS_I2C_PACKED_DFSRR  0x02
#define FS_I2C_SEPARATE_DFSRR 0x04

quoted

Are you sure ? If I don't set the BNBE (Bus Not Busy Enable) bit, I just
get timeouts.

quoted

From the manual it looks as if setting BNBE might cause extra

interrupts, which the driver has no way to handle.  Could you try
enabling the interrupts, and see if this happens?

quoted

Yes sure, that's the easiest way. It's just that I'd like to avoid it.
Especially when it's content is dependent on if the user has choosed to
use irq or not.
But It's sure is a pity that the register is shared between the two I2C
... Because even with a flag, the driver should be passed the address of
this register, and what bits to use.

How about putting a function pointer for platform interrupt enabling and
disabling into the ocp_fs_i2c_data?

Well, you're right setting the BNBE is not right, it just hang because
interrupts are fired and not handled.
Now, here is the interesting part :
If I set the ocp_def to use both I2C controller, defining the I2C1
before I2C2, it works fine. Now if I just invert the entry, it does not.
Interrupts for I2C2 are never fired.
If I only put I2C1, it seems to work ( no timout). With only I2C2,
interrupts are never fired ...

I have no clue why ! The best option is just to set IRQ to OCP_IRQ_NA,
and it works fine, whatever the value of the shared interrupt registers is.

It looks to me that supporting the 5200 will require a lot of small
changes.

Well, for the interrupt yes that a quirk. The solution of dropping
interrupt completly for it is the best option for now.
For the DFSRR register, the solution mentionned above is clean, the 3
chips requires different implementation anyway.
For the clock thing, I think it's good to be able to set the I2C clock.


Sylvain Munaut


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/