Re: [PATCH] i2c: adds support for i2c bus on 8xx

On Thu, 10 May 2007 11:28:35 +0200
Jean Delvare wrote:

Hi Vitaly,

There is a mailing list dedicated to Linux I2C development (check
MAINTAINERS), so why don't you use it instead of the already cluttered
LKML?

because it's both arch-specific and drivers/ thing. I'll Cc that ML next time.

On Tue, 08 May 2007 10:31:31 +0400, Vitaly Bordug wrote:
Utilized devicetree to store I2C data, ported i2c-algo-8xx.c from
2.4 approach(which remains nearly intact), refined i2c-rpx.c. I2C
functionality has been validated on mpc885ads with EEPROM access.

Interface has been reworked to of_device as well as other feedback
addressed. Repeated start ability is missing from the
implementation.

Did you check the hardware documentation? Is is a hardware limitation?
It is likely to cause problems, so it should be investigated, and if
it really cannot be fixed, then this must be clearly documented (in
Kconfig and in the driver) and the advertised functionalities of the
driver should be reduced accordingly (most SMBus transactions include
a repeated start.)

Yes, I've checked RM about I2C controller, and cannot find anything about repeated
start. There are a few words about multi-master considerations, that SoC features bus arbitration,
but to avoid collisions, "higher-level handshake protocol must be used" (MPC885 RM, p. 32-6).

How the driver should advertise this case, can you suggest?


Signed-off-by: Vitaly Bordug <vitb@xxxxxxxxxxxxxxxxxxx>

---

arch/powerpc/platforms/8xx/mpc885ads_setup.c | 15 +
arch/powerpc/sysdev/fsl_soc.c | 15 +
drivers/i2c/algos/Kconfig | 2
drivers/i2c/algos/Makefile | 1
drivers/i2c/algos/i2c-algo-8xx.c | 520
++++++++++++++++++++++++++
drivers/i2c/busses/Kconfig | 2
drivers/i2c/busses/i2c-rpx.c | 143 +++++--
include/linux/i2c-algo-8xx.h | 29 + 8 files
changed, 683 insertions(+), 44 deletions(-)

After my original review, we had agreed that having a separate
algorithm driver wasn't needed. You didn't change that though. Why? I
will not consider applying this patch until that change is done (or a
good reason not to do it is given.)


yes, that's correct, missed this point...

diff --git a/arch/powerpc/platforms/8xx/mpc885ads_setup.c
b/arch/powerpc/platforms/8xx/mpc885ads_setup.c index
a57b577..40d12a2 100644 ---
a/arch/powerpc/platforms/8xx/mpc885ads_setup.c +++
b/arch/powerpc/platforms/8xx/mpc885ads_setup.c @@ -51,6 +51,17 @@
static void init_smc1_uart_ioports(struct fs_uart_platform_info*
fpi); static void init_smc2_uart_ioports(struct
fs_uart_platform_info* fpi); static void init_scc3_ioports(struct
fs_platform_info* ptr); +#ifdef CONFIG_I2C_RPXLITE
+static void init_i2c_ioports()
+{
+ cpm8xx_t *cp = (cpm8xx_t *)immr_map(im_cpm);
+
+ setbits32(&cp->cp_pbpar, 0x00000030);
+ setbits32(&cp->cp_pbdir, 0x00000030);
+ setbits16(&cp->cp_pbodr, 0x0030);
+}
+#endif
+
void __init mpc885ads_board_setup(void)
{
cpm8xx_t *cp;
@@ -115,6 +126,10 @@ void __init mpc885ads_board_setup(void)
immr_unmap(io_port);

#endif
+
+#ifdef CONFIG_I2C_RPXLITE
+ init_i2c_ioports();
+#endif
}


diff --git a/arch/powerpc/sysdev/fsl_soc.c
b/arch/powerpc/sysdev/fsl_soc.c index 8a123c7..be60db7 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -33,6 +33,7 @@
#include <asm/irq.h>
#include <asm/time.h>
#include <asm/prom.h>
+#include <asm/of_platform.h>
#include <sysdev/fsl_soc.h>
#include <mm/mmu_decl.h>
#include <asm/cpm2.h>
@@ -1102,4 +1103,18 @@ err:

arch_initcall(cpm_smc_uart_of_init);

+static int __init fsl_i2c_cpm_of_init(void)
+{
+ struct device_node *np = NULL;
+ /*
+ * Register all the devices which type is "i2c-cpm"
+ */
+ while ((np = of_find_compatible_node(np, "i2c",
"fsl,i2c-cpm")) != NULL)
+ of_platform_device_create(np, "fsl-i2c-cpm", NULL);
+ return 0;
+}
+
+arch_initcall(fsl_i2c_cpm_of_init);
+
+
#endif /* CONFIG_8xx */
diff --git a/drivers/i2c/algos/Kconfig b/drivers/i2c/algos/Kconfig
index af02034..11e37ff 100644
--- a/drivers/i2c/algos/Kconfig
+++ b/drivers/i2c/algos/Kconfig
@@ -41,6 +41,8 @@ config I2C_ALGOPCA
config I2C_ALGO8XX
tristate "MPC8xx CPM I2C interface"
depends on 8xx && I2C
+ help
+ 8xx I2C Algorithm,supports the CPM I2C interface for
mpc8xx CPUs.
config I2C_ALGO_SGI
tristate "I2C SGI interfaces"
diff --git a/drivers/i2c/algos/Makefile b/drivers/i2c/algos/Makefile
index cac1051..1bd3b37 100644
--- a/drivers/i2c/algos/Makefile
+++ b/drivers/i2c/algos/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_ALGOBIT) += i2c-algo-bit.o
obj-$(CONFIG_I2C_ALGOPCF) += i2c-algo-pcf.o
obj-$(CONFIG_I2C_ALGOPCA) += i2c-algo-pca.o
obj-$(CONFIG_I2C_ALGO_SGI) += i2c-algo-sgi.o
+obj-$(CONFIG_I2C_ALGO8XX) += i2c-algo-8xx.o

ifeq ($(CONFIG_I2C_DEBUG_ALGO),y)
EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/i2c/algos/i2c-algo-8xx.c
b/drivers/i2c/algos/i2c-algo-8xx.c new file mode 100644
index 0000000..01ffcee
--- /dev/null
+++ b/drivers/i2c/algos/i2c-algo-8xx.c
@@ -0,0 +1,520 @@
+/*
+ * i2c-algo-8xx.c i2x driver algorithms for MPC8XX CPM
+ * Copyright (c) 1999 Dan Malek (dmalek@xxxxxxx).
+ *
+ This program is free software; you can redistribute it and/or
modify
+ it under the terms of the GNU General Public License as
published by
+ the Free Software Foundation; either version 2 of the License,
or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public
License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * moved into proper i2c interface; separated out platform specific
+ * parts into i2c-rpx.c
+ * Brad Parker (brad@xxxxxxxxxxx)
+ *
+ * (C) 2007 Montavista Software, Inc.
+ * Vitaly Bordug <vitb@xxxxxxxxxxxxxxxxxxx>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/sched.h>
+#include <linux/i2c.h>
+#include <linux/i2c-algo-8xx.h>
+#include <asm/io.h>
+#include <asm/cacheflush.h>
+#include <asm/time.h>
+#include <asm/mpc8xx.h>
+
+/* Try to define this if you have an older CPU (earlier than rev
D4) */ +#undef I2C_CHIP_ERRATA
+
+static wait_queue_head_t iic_wait;
+static ushort r_tbase, r_rbase;
+
+static int cpm_debug;
+
+static irqreturn_t cpm_iic_interrupt(int irq, void *dev_id)
+{
+ i2c8xx_t *i2c = (i2c8xx_t *) dev_id;
+
+#if 0
+ /* Chip errata, clear enable. This is not needed on rev D4
CPUs */
+ /* This should probably be removed and replaced by
I2C_CHIP_ERRATA stuff */
+ /* Someone with a buggy CPU needs to confirm that */
+ out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | ~1);
+#endif
+ /* Clear interrupt.
+ */
+ out_8(&i2c->i2c_i2cer, 0xff);
+
+ /* Get 'me going again.
+ */
+ wake_up_interruptible(&iic_wait);
+
+ return IRQ_HANDLED;
+}
+
+static int cpm_iic_init(struct i2c_adapter *adap)
+{
+ struct i2c_algo_8xx_data *cpm = adap->algo_data;
+ iic_t *iip = cpm->iip;
+ i2c8xx_t *i2c = cpm->i2c;
+ unsigned char brg;
+
+ if (cpm_debug)
+ dev_dbg(adap->dev, "cpm_iic_init()\n");

Should be "&adap->dev". Same for all other calls. This means you never
tried to compile your code with debugging enabled. Please do (there
are options in the I2C Support menu for this.)

OK

+
+ init_waitqueue_head(&iic_wait);
+
+ /* Initialize the parameter ram.
+ * We need to make sure many things are initialized to
zero,
+ * especially in the case of a microcode patch.
+ */
+ iip->iic_rstate = 0;
+ iip->iic_rdp = 0;
+ iip->iic_rbptr = 0;
+ iip->iic_rbc = 0;
+ iip->iic_rxtmp = 0;
+ iip->iic_tstate = 0;
+ iip->iic_tdp = 0;
+ iip->iic_tbptr = 0;
+ iip->iic_tbc = 0;
+ iip->iic_txtmp = 0;
+
+ /* Set up the IIC parameters in the parameter ram.
+ */
+ iip->iic_tbase = r_tbase = cpm->dp_addr;
+ iip->iic_rbase = r_rbase = cpm->dp_addr + sizeof(cbd_t) *
2; +
+ if (cpm_debug) {
+ dev_dbg(adap->dev, "iip %p, dp_addr 0x%x\n",
cpm->iip,
+ cpm->dp_addr);
+ dev_dbg(adap->dev, "iic_tbase %d, r_tbase %d\n",
iip->iic_tbase,
+ r_tbase);
+ }
+
+ iip->iic_tfcr = SMC_EB;
+ iip->iic_rfcr = SMC_EB;
+
+ /* Set maximum receive size.
+ */
+ iip->iic_mrblr = CPM_MAX_READ;
+
+ /* Initialize Tx/Rx parameters.
+ */
+ if (cpm->reloc == 0) {
+ cpm8xx_t *cp = cpm->cp;
+ int res;
+
+ u16 v = mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_INIT_TRX)
| CPM_CR_FLG; +
+ out_be16(&cp->cp_cpcr, v);
+ res = wait_event_timeout(iic_wait,
+ !(in_be16(&cp->cp_cpcr) &
CPM_CR_FLG),
+ HZ * 10);

This is a pretty long timeout. Can it realistically take that long?

In fact, it can. Especially when CPM is loaded with Ethernet/UART/Etc transactions. If you feel it is overkill
and I2C shouldn't do that, I'll trim it down though.

+ if (!res)
+ return -EIO;
+
+ } else {
+ iip->iic_rbptr = iip->iic_rbase;
+ iip->iic_tbptr = iip->iic_tbase;
+ iip->iic_rstate = 0;
+ iip->iic_tstate = 0;
+ }
+
+ /* Select an arbitrary address. Just make sure it is
unique.
+ */
+ out_8(&i2c->i2c_i2add, 0xfe);
+
+ /* Make clock run at 60 kHz.
+ */
+ brg = ppc_proc_freq / (32 * 2 * 60000) - 3;
+ out_8(&i2c->i2c_i2brg, brg);
+
+ out_8(&i2c->i2c_i2mod, 0x00);
+ out_8(&i2c->i2c_i2com, 0x01); /* Master mode */
+
+ /* Disable interrupts.
+ */
+ out_8(&i2c->i2c_i2cmr, 0);
+ out_8(&i2c->i2c_i2cer, 0xff);
+
+ /* Install interrupt handler.
+ */
+ request_irq(cpm->irq, cpm_iic_interrupt, 0, "8xx_i2c",
i2c); +
+ return 0;
+}
+
+static int cpm_iic_shutdown(struct i2c_algo_8xx_data *cpm)
+{
+ i2c8xx_t *i2c = cpm->i2c;
+
+ /* Shut down IIC.
+ */
+ out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | ~1);
+ out_8(&i2c->i2c_i2cmr, 0);
+ out_8(&i2c->i2c_i2cer, 0xff);
+
+ return (0);
+}

Should return void, it cannot fail.

OK
+
+static void cpm_reset_iic_params(iic_t * iip)
+{
+ iip->iic_tbase = r_tbase;
+ iip->iic_rbase = r_rbase;
+
+ iip->iic_tfcr = SMC_EB;
+ iip->iic_rfcr = SMC_EB;
+
+ iip->iic_mrblr = CPM_MAX_READ;
+
+ iip->iic_rstate = 0;
+ iip->iic_rdp = 0;
+ iip->iic_rbptr = iip->iic_rbase;
+ iip->iic_rbc = 0;
+ iip->iic_rxtmp = 0;
+ iip->iic_tstate = 0;
+ iip->iic_tdp = 0;
+ iip->iic_tbptr = iip->iic_tbase;
+ iip->iic_tbc = 0;
+ iip->iic_txtmp = 0;
+}
+
+#define BD_SC_NAK ((ushort)0x0004) /* NAK -
did not respond */ +#define BD_SC_OV
((ushort)0x0002) /* OV - receive overrun */ +#define
CPM_CR_CLOSE_RXBD ((ushort)0x0007) +
+static void force_close(struct i2c_adapter *adap)
+{
+ struct i2c_algo_8xx_data *cpm = adap->algo_data;
+ i2c8xx_t *i2c = cpm->i2c;
+ if (cpm->reloc == 0) { /* micro code disabled */
+ cpm8xx_t *cp = cpm->cp;
+ u16 v =
+ mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_CLOSE_RXBD) |
CPM_CR_FLG; +
+ if (cpm_debug)
+ printk("force_close()\n");
+
+ out_be16(&cp->cp_cpcr, v);
+ wait_event_timeout(iic_wait,
+ !(in_be16(&cp->cp_cpcr) &
CPM_CR_FLG),
+ HZ * 5);
+ }
+ out_8(&i2c->i2c_i2cmr, 0x00); /* Disable all
interrupts */
+ out_8(&i2c->i2c_i2cer, 0xff);
+}
+
+/* Read from IIC...
+ * abyte = address byte, with r/w flag already set
+ */
+static int
+cpm_iic_read(struct i2c_adapter *adap, u_char abyte, char *buf,
int count) +{
+ struct i2c_algo_8xx_data *cpm = adap->algo_data;
+ iic_t *iip = cpm->iip;
+ i2c8xx_t *i2c = cpm->i2c;
+ cbd_t *tbdf, *rbdf;
+ u_char *tb;
+ int res = 0;
+
+ if (count >= CPM_MAX_READ)
+ return -EINVAL;
+
+ /* check for and use a microcode relocation patch */
+ if (cpm->reloc) {
+ cpm_reset_iic_params(iip);
+ }
+
+ tbdf = (cbd_t *) cpm_dpram_addr(iip->iic_tbase);
+ rbdf = (cbd_t *) cpm_dpram_addr(iip->iic_rbase);
+
+ /* To read, we need an empty buffer of the proper length.
+ * All that is used is the first byte for address, the
remainder
+ * is just used for timing (and doesn't really have to
exist).
+ */
+ tb = cpm->temp;
+ tb = (u_char *) (((uint) tb + 15) & ~15);
+ tb[0] = abyte; /* Device address byte w/rw
flag */ +
+ flush_dcache_range((unsigned long)tb, (unsigned long)(tb +
1)); +
+ if (cpm_debug)
+ dev_dbg(adap->dev, "cpm_iic_read(abyte=0x%x)\n",
abyte); +
+ tbdf->cbd_bufaddr = __pa(tb);
+ tbdf->cbd_datlen = count + 1;
+ tbdf->cbd_sc = BD_SC_READY | BD_SC_LAST | BD_SC_WRAP |
BD_IIC_START; +
+ iip->iic_mrblr = count + 1; /* prevent excessive
read, +1
+ is needed otherwise
will the
+ RXB interrupt come too
early */ +
+ /* flush will invalidate too. */
+ flush_dcache_range((unsigned long)buf, (unsigned long)(buf
+ count)); +
+ rbdf->cbd_datlen = 0;
+ rbdf->cbd_bufaddr = __pa(buf);
+ rbdf->cbd_sc = BD_SC_EMPTY | BD_SC_WRAP | BD_SC_INTRPT;
+
+ /* Chip bug, set enable here */
+ out_8(&i2c->i2c_i2cmr, 0x13); /* Enable some
interupts */
+ out_8(&i2c->i2c_i2cer, 0xff);
+ out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) |
1); /* Enable */
+ out_8(&i2c->i2c_i2com, in_8(&i2c->i2c_i2com) |
0x80); /* Begin transmission */ +
+ /* Wait for IIC transfer */
+ res = wait_event_interruptible_timeout(iic_wait, 0, 1 *
HZ); +
+ if (res < 0) {

The usual style is no blank line between the action and the test.

OK

[snip]
pmsg->len,
+ (unsigned long)pmsg->buf);
+
+ addr = pmsg->addr << 1;
+ if (pmsg->flags & I2C_M_RD)
+ addr |= 1;
+ if (pmsg->flags & I2C_M_REV_DIR_ADDR)
+ addr ^= 1;

You didn't drop this?

Correct, missed the cleanup.
+
+ if (pmsg->flags & I2C_M_RD) {
+ /* read bytes into buffer */
+ ret = cpm_iic_read(adap, addr, pmsg->buf,
pmsg->len);
+ if (cpm_debug)
+ dev_dbg(adap->dev,
+ "i2c-algo-8xx.o: read %d
bytes\n", ret);
+ if (ret < pmsg->len) {
+ return (ret < 0) ? ret :
-EREMOTEIO;
+ }
+ } else {
+ /* write bytes from buffer */
+ ret = cpm_iic_write(adap, addr, pmsg->buf,
pmsg->len);
+ if (cpm_debug)
+ dev_dbg(adap->dev,
"i2c-algo-8xx.o: wrote %d\n",
+ ret);
+ if (ret < pmsg->len) {
+ return (ret < 0) ? ret :
-EREMOTEIO;
+ }
+ }
+ }
+ return (num);

Useless parentheses.

yeah...
+}
+
+static u32 cpm_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+/* -----exported algorithm data:
------------------------------------- */ +
+static struct i2c_algorithm cpm_algo = {
+ .master_xfer = cpm_xfer,
+ .functionality = cpm_func,
+};
+
+/*
+ * registering functions to load algorithms at runtime
+ */
+int i2c_8xx_add_bus(struct i2c_adapter *adap)
+{
+ int res;
+
+ if (cpm_debug)
+ dev_dbg(adap->dev,
+ "i2c-algo-8xx.o: hw routines for %s
registered.\n",
+ adap->name);
+
+ /* register new adapter to i2c module... */
+
+ adap->algo = &cpm_algo;
+
+ res = cpm_iic_init(adap);
+
+ if (res)
+ return res;
+
+ return i2c_add_adapter(adap);
+}
+
+int i2c_8xx_del_bus(struct i2c_adapter *adap)
+{
+ struct i2c_algo_8xx_data *cpm = adap->algo_data;
+
+ i2c_del_adapter(adap);
+
+ return cpm_iic_shutdown(cpm);
+}

Should return void.

OK.
+
+EXPORT_SYMBOL(i2c_8xx_add_bus);
+EXPORT_SYMBOL(i2c_8xx_del_bus);
+
+MODULE_AUTHOR("Brad Parker <brad@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("I2C-Bus MPC8XX algorithm");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index ece31d2..df29fb5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -365,7 +365,7 @@ config I2C_PROSAVAGE

config I2C_RPXLITE
tristate "Embedded Planet RPX Lite/Classic support"
- depends on (RPXLITE || RPXCLASSIC) && I2C
+ depends on (RPXLITE || RPXCLASSIC || MPC86XADS ||
MPC885ADS) && I2C select I2C_ALGO8XX

A recent change to the i2c Kconfig files make it no longer needed to
have all options depend on I2C explicitly. When you regenerate your
patch on top of Linus' latest tree, you'll have to update this.

The help text for this option is still missing.


OK, good point.

[snip]
Question: once this is merged, will we possibly get rid of
arch/ppc/boot/simple/iic.c?

yes of course. In fact, this layer being obsoleted by powerpc works,
and may be broken in numerous ways (I am regularly asked why make fail somewhere in arch/ppc/boot,
and common ans is just to use uImage that does not compile this part).

There still may be boards that neither have u-boot as firmware, nor able to update what's existing. I think
declare stuff as feature to be removed would be enough for concerned folks to react.
OTOH, there's good chance that nobody will even care :)


--
Sincerely, Vitaly

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages