Using the port of 2.4 code from Vitaly Bordug <[email protected]>
and the actual algorithm used by the i2c driver of the DBox code on
cvs.tuxboc.org from Tmbinc, Gillem ([email protected]). Renamed
i2c-algo-8xx.c to i2c-algo-cpm.c and i2c-rpx.c to i2c-8xx.c. Added
original i2c-rpx.c as i2c-8xx-ppc.c for pre-OF (arch ppc) devices.
Signed-off-by: Jochen Friedrich <[email protected]>
---
This can be pulled from git://git.bocc.de/dbox2.git i2c
arch/powerpc/boot/dts/mpc885ads.dts | 9 +
arch/powerpc/platforms/8xx/mpc885ads_setup.c | 6 +
arch/powerpc/sysdev/fsl_soc.c | 61 +++-
drivers/i2c/algos/Kconfig | 12 +
drivers/i2c/algos/Makefile | 1 +
drivers/i2c/algos/i2c-algo-cpm.c | 563 ++++++++++++++++++++++++++
drivers/i2c/busses/Kconfig | 22 +
drivers/i2c/busses/Makefile | 2 +
drivers/i2c/busses/i2c-8xx-ppc.c | 105 +++++
drivers/i2c/busses/i2c-8xx.c | 170 ++++++++
include/linux/i2c-algo-cpm.h | 34 ++
11 files changed, 984 insertions(+), 1 deletions(-)
create mode 100644 drivers/i2c/algos/i2c-algo-cpm.c
create mode 100644 drivers/i2c/busses/i2c-8xx-ppc.c
create mode 100644 drivers/i2c/busses/i2c-8xx.c
create mode 100644 include/linux/i2c-algo-cpm.h
On Mon, Oct 15, 2007 at 01:10:12PM +0200, Jochen Friedrich wrote:
> Using the port of 2.4 code from Vitaly Bordug <[email protected]>
> and the actual algorithm used by the i2c driver of the DBox code on
> cvs.tuxboc.org from Tmbinc, Gillem ([email protected]). Renamed
> i2c-algo-8xx.c to i2c-algo-cpm.c and i2c-rpx.c to i2c-8xx.c. Added
> original i2c-rpx.c as i2c-8xx-ppc.c for pre-OF (arch ppc) devices.
Do we really need to be adding features for arch/ppc at this point? It'll
be going away in June. arch/ppc-specific things outside of arch/ppc itself
will also be more likely to be missed in the removal.
Also, please post inline rather than as an attachment; attachments are
harder to quote in a reply.
> diff --git a/arch/powerpc/boot/dts/mpc885ads.dts b/arch/powerpc/boot/dts/mpc885ads.dts
> index 8848e63..a526c02 100644
> --- a/arch/powerpc/boot/dts/mpc885ads.dts
> +++ b/arch/powerpc/boot/dts/mpc885ads.dts
> @@ -213,6 +213,15 @@
> fsl,cpm-command = <0080>;
> linux,network-index = <2>;
> };
> +
> + i2c@860 {
> + device_type = "i2c";
No device_type.
> + compatible = "fsl-i2c-cpm";
Should be fsl,cpm-i2c. Is cpm2 i2c the same? If not, it should be
fsl,cpm1-i2c. It's probably best to specify it anyway, along with
fsl,mpc885-i2c.
> diff --git a/arch/powerpc/platforms/8xx/mpc885ads_setup.c b/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> index 2cf1b6a..350018b 100644
> --- a/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> +++ b/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> @@ -175,6 +175,12 @@ static void __init init_ioports(void)
>
> /* Set FEC1 and FEC2 to MII mode */
> clrbits32(&mpc8xx_immr->im_cpm.cp_cptr, 0x00000180);
> +
> +#ifdef CONFIG_I2C_8XX
> + setbits32(&mpc8xx_immr->im_cpm.cp_pbpar, 0x00000030);
> + setbits32(&mpc8xx_immr->im_cpm.cp_pbdir, 0x00000030);
> + setbits16(&mpc8xx_immr->im_cpm.cp_pbodr, 0x0030);
> +#endif
Please add this to mpc885ads_pins, rather than poking the registers
directly. The relevant lines are:
{CPM_PORTB, 26, CPM_PIN_OUTPUT},
{CPM_PORTB, 27, CPM_PIN_OUTPUT},
> +static const char *i2c_regs = "regs";
> +static const char *i2c_pram = "pram";
> +static const char *i2c_irq = "interrupt";
> +
> +static int __init fsl_i2c_cpm_of_init(void)
> +{
> + struct device_node *np;
> + unsigned int i;
> + struct platform_device *i2c_dev;
> + int ret;
> +
> + for (np = NULL, i = 0;
> + (np = of_find_compatible_node(np, "i2c", "fsl-i2c-cpm")) != NULL;
> + i++) {
Why not just make an of_platform device instead of this glue code?
> diff --git a/drivers/i2c/algos/Kconfig b/drivers/i2c/algos/Kconfig
> index 014dfa5..7a8200e 100644
> --- a/drivers/i2c/algos/Kconfig
> +++ b/drivers/i2c/algos/Kconfig
> @@ -14,6 +14,18 @@ config I2C_ALGOBIT
> This support is also available as a module. If so, the module
> will be called i2c-algo-bit.
>
> +config I2C_ALGOCPM
> + tristate "I2C MPC8xx CPM and MPC8260 CPM2 interfaces"
> + depends on ( 8xx || 8260 ) && I2C
> + help
> + CPM I2C Algorithm, supports the CPM I2C interface for mpc8xx
> + and mpc8260 CPUs. Say Y if you own a CPU of this class
What if I'm just borrowing it? :-)
> +static irqreturn_t cpm_iic_interrupt(int irq, void *dev_id)
> +{
> + struct i2c_adapter *adap;
> + struct i2c_algo_cpm_data *cpm;
> + i2c8xx_t *i2c;
> + int i;
> +
> + adap = (struct i2c_adapter *) dev_id;
> + cpm = adap->algo_data;
> + i2c = cpm->i2c;
> +
> + /* Clear interrupt.
> + */
> + i = in_8(&i2c->i2c_i2cer);
> + out_8(&i2c->i2c_i2cer, i);
> +
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "Interrupt: %x\n", i);
> +
> + /* Get 'me going again.
> + */
> + wake_up_interruptible(&cpm->iic_wait);
> +
> + return IRQ_HANDLED;
> +}
Should only return IRQ_HANDLED if the event register was non-zero.
> +static int cpm_iic_init(struct i2c_adapter *adap)
> +{
> + struct i2c_algo_cpm_data *cpm = adap->algo_data;
> + iic_t *iip = cpm->iip;
> + i2c8xx_t *i2c = cpm->i2c;
> + unsigned char brg;
> + int ret, i;
> +
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "cpm_iic_init()\n");
Can you fold the if statement into a macro? Or just do a raw dev_dbg with
no test, like most drivers do.
> + ret = 0;
> + init_waitqueue_head(&cpm->iic_wait);
> + mutex_init(&cpm->iic_mutex);
> + /* Initialize the parameter ram.
> + * We need to make sure many things are initialized to zero,
> + * especially in the case of a microcode patch.
> + */
> + out_be32(&iip->iic_rstate, 0);
> + out_be32(&iip->iic_rdp, 0);
> + out_be16(&iip->iic_rbptr, 0);
> + out_be16(&iip->iic_rbc, 0);
> + out_be32(&iip->iic_rxtmp, 0);
> + out_be32(&iip->iic_tstate, 0);
> + out_be32(&iip->iic_tdp, 0);
> + out_be16(&iip->iic_tbptr, 0);
> + out_be16(&iip->iic_tbc, 0);
> + out_be32(&iip->iic_txtmp, 0);
This appears to be done twice, here and in cpm_reset_iic_params.
> + u16 v = mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_INIT_TRX) | CPM_CR_FLG;
Should use fsl,cpm-command rather than hardcoded constants.
> + /* Select an arbitrary address. Just make sure it is unique.
> + */
> + out_8(&i2c->i2c_i2add, 0xfe);
It's a 7-bit address... and are you sure that 0x7e is unique? Does this
driver even support slave operation?
> + /* Make clock run at 60 kHz.
> + */
> + /* brg = ppc_proc_freq / (32 * 2 * 60000) - 3; */
> + brg = 7;
> + out_8(&i2c->i2c_i2brg, brg);
Why is this hardcoded?
> + 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);
> +
> + /* Allocate TX and RX buffers */
> + for (i = 0; i < CPM_MAXBD; i++) {
> + cpm->rxbuf[i] = (unsigned char *)dma_alloc_coherent(
> + NULL, CPM_MAX_READ + 1, &cpm->rxdma[i], GFP_KERNEL);
> + if (!cpm->rxbuf[i]) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + cpm->txbuf[i] = (unsigned char *)dma_alloc_coherent(
> + NULL, CPM_MAX_READ + 1, &cpm->txdma[i], GFP_KERNEL);
> + if (!cpm->txbuf[i]) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + }
> +
> + /* Install interrupt handler.
> + */
> + if (request_irq(cpm->irq, cpm_iic_interrupt, 0, "8xx_i2c", adap)) {
cpm-i2c, not 8xx.
> + ret = -EIO;
> + goto out;
> + }
> +
> + return 0;
> +out:
> + for (i = 0; i < CPM_MAXBD; i++) {
> + if (cpm->rxbuf[i]) dma_free_coherent(NULL, CPM_MAX_READ + 1,
> + cpm->rxbuf[i], cpm->rxdma[i]);
> + if (cpm->txbuf[i]) dma_free_coherent(NULL, CPM_MAX_READ + 1,
> + cpm->txbuf[i], cpm->txdma[i]);
> + }
Please put a newline between the if test and the if body.
> +static void force_close(struct i2c_adapter *adap)
> +{
> + struct i2c_algo_cpm_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;
Why only if there's no microcode relocation?
> +static void cpm_parse_message(struct i2c_adapter *adap, struct i2c_msg *pmsg,
> + int num, int tx, int rx)
> +{
> + cbd_t *tbdf, *rbdf;
> + u_char addr;
> + u_char *tb;
> + u_char *rb;
> + struct i2c_algo_cpm_data *cpm = adap->algo_data;
> + iic_t *iip = cpm->iip;
> + int i, dscan;
> +
> + tbdf = (cbd_t *) cpm_dpram_addr(in_be16(&iip->iic_tbase));
> + rbdf = (cbd_t *) cpm_dpram_addr(in_be16(&iip->iic_rbase));
> +
> + /* This chip can't do zero lenth writes. However, the i2c core uses
s/lenth/length/
> + if (pmsg->flags & I2C_M_RD) {
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "rx sc %04x, rx sc %04x\n",
> + tbdf[tx].cbd_sc, rbdf[rx].cbd_sc);
> + if (tbdf[tx].cbd_sc & BD_SC_NAK) {
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "IIC read; no ack\n");
> + if (pmsg->flags & I2C_M_IGNORE_NAK)
> + return 0;
> + else
> + return -EREMOTEIO;
s/EREMOTEIO/EIO/
> +static int cpm_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct i2c_algo_cpm_data *cpm = adap->algo_data;
> + i2c8xx_t *i2c = cpm->i2c;
> + iic_t *iip = cpm->iip;
> + struct i2c_msg *pmsg, *rmsg;
> + int ret, i;
> + int tptr;
> + int rptr;
> + cbd_t *tbdf, *rbdf;
> +
> + if (num > CPM_MAXBD)
> + return -EREMOTEIO;
> +
> + /* Check if we have any oversized READ requests */
> + for (i = 0; i < num; i++) {
> + pmsg = &msgs[i];
> + if (pmsg->len >= CPM_MAX_READ)
> + return -EREMOTEIO;
> + }
s/EREMOTEIO/EINVAL/
> +
> + mutex_lock(&cpm->iic_mutex);
> +
> + /* check for and use a microcode relocation patch */
> + if (cpm->reloc)
> + cpm_reset_iic_params(cpm);
On every transfer?
> + while (tptr < num) {
> + /* Check for outstanding messages */
> + dev_dbg(&adap->dev, "test ready.\n");
> + if (!(tbdf[tptr].cbd_sc & BD_SC_READY)) {
> + dev_dbg(&adap->dev, "ready.\n");
> + rmsg = &msgs[tptr];
> + ret = cpm_check_message(adap, rmsg, tptr, rptr);
> + tptr++;
> + if (rmsg->flags & I2C_M_RD)
> + rptr++;
> + if (ret) {
> + force_close(adap);
> + mutex_unlock(&cpm->iic_mutex);
> + return -EREMOTEIO;
s/-EREMOTEIO/ret/
> +config I2C_8XX
> + tristate "MPC8xx CPM with Open Firmware"
It's not really Open Firmware... just a flat device tree.
> diff --git a/drivers/i2c/busses/i2c-8xx.c b/drivers/i2c/busses/i2c-8xx.c
> new file mode 100644
> index 0000000..c38b52e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-8xx.c
> @@ -0,0 +1,170 @@
> +/*
> + * Embedded Planet RPX Lite MPC8xx CPM I2C interface.
> + * Copyright (c) 1999 Dan Malek ([email protected]).
> + *
> + * moved into proper i2c interface;
> + * Brad Parker ([email protected])
> + *
> + * (C) 2007 Montavista Software, Inc.
> + * Vitaly Bordug <[email protected]>
> + *
> + * RPX lite specific parts of the i2c interface
> + * Update: There actually isn't anything RPXLite-specific about this module.
> + * This should work for most any 8xx board. The console messages have been
> + * changed to eliminate RPXLite references.
So let's change the title at the top of the file...
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/stddef.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-cpm.h>
> +#include <linux/of_device.h>
Why are you including platform_device, and running glue code in fsl_soc.c,
if this is an of_platform driver?
> +#include <linux/of_platform.h>
> +#include <asm/mpc8xx.h>
> +#include <asm/commproc.h>
> +#include <asm/fs_pd.h>
> +
> +
> +struct m8xx_i2c {
> + char *base;
> + struct of_device *ofdev;
> + struct i2c_adapter adap;
> + struct i2c_algo_cpm_data *algo_8xx;
> +};
> +
> +static struct i2c_algo_cpm_data m8xx_data;
> +
> +static const struct i2c_adapter m8xx_ops = {
> + .owner = THIS_MODULE,
> + .name = "i2c-8xx",
> + .id = I2C_HW_MPC8XX_EPON,
> + .algo_data = &m8xx_data,
> + .dev.parent = &platform_bus,
> + .class = I2C_CLASS_HWMON,
> +};
It's not on the platform bus; it's on the soc of_platform bus. Why is this
embedded in the i2c_adapter struct anyway? i2c_adapter should hold a
pointer to whatever the probe function gives you.
> + data->irq = irq_of_parse_and_map(np, 0);
> + if (data->irq < 0)
> + return -EINVAL;
> +
> + if (of_address_to_resource(np, 1, &r))
> + return -EINVAL;
> +
> + data->iip = ioremap(r.start, r.end - r.start + 1);
> + if (data->iip == NULL)
> + return -EINVAL;
> +
> + /* Check for and use a microcode relocation patch.
> + */
> + data->reloc = data->iip->iic_rpbase;
> + if (data->reloc)
> + data->iip = (iic_t *)&cp->cp_dpmem[data->iip->iic_rpbase];
> +
> + if (of_address_to_resource(np, 0, &r))
> + return -EINVAL;
> +
> + data->i2c = ioremap(r.start, r.end - r.start + 1);
Use of_iomap.
> + if (data->i2c == NULL)
> + return -EINVAL;
> +
> + /* Allocate space for two transmit and two receive buffer
> + * descriptors in the DP ram.
> + */
> + data->dp_addr = cpm_dpalloc(sizeof(cbd_t) * 4, 8);
Please use the new muram_dpalloc name.
> + if (!data->dp_addr)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int i2c_8xx_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + int result;
> + struct m8xx_i2c *i2c;
> +
> + i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
> +
> + i2c->ofdev = ofdev;
> + i2c->algo_8xx = &m8xx_data;
> +
> + m8xx_iic_init(i2c);
> +
> + dev_set_drvdata(&ofdev->dev, i2c);
> +
> + i2c->adap = m8xx_ops;
> + i2c_set_adapdata(&i2c->adap, i2c);
> + i2c->adap.dev.parent = &ofdev->dev;
> +
> + result = i2c_cpm_add_bus(&i2c->adap);
> + if (result < 0) {
> + printk(KERN_ERR "i2c-8xx: Unable to register with I2C\n");
> + kfree(i2c);
> + }
> +
> + return result;
> +}
> +
> +static int i2c_8xx_remove(struct of_device *ofdev)
> +{
> + struct m8xx_i2c *i2c = dev_get_drvdata(&ofdev->dev);
> +
> + i2c_cpm_del_bus(&i2c->adap);
> + dev_set_drvdata(&ofdev->dev, NULL);
> +
> + kfree(i2c);
> + return 0;
> +}
> +
> +static struct of_device_id i2c_8xx_match[] = {
> + {
> + .type = "i2c",
> + .compatible = "fsl,i2c-cpm",
> + },
> + {},
> +};
> +
Why is an 8xx driver matching all i2c cpm (i.e. what about cpm2)?
-Scot
Hi Scott,
> Do we really need to be adding features for arch/ppc at this point? It'll
> be going away in June. arch/ppc-specific things outside of arch/ppc itself
> will also be more likely to be missed in the removal.
>
> Also, please post inline rather than as an attachment; attachments are
> harder to quote in a reply.
>
OK. I'll remove the ARC=ppc parts.
>> diff --git a/arch/powerpc/boot/dts/mpc885ads.dts b/arch/powerpc/boot/dts/mpc885ads.dts
>> index 8848e63..a526c02 100644
>> --- a/arch/powerpc/boot/dts/mpc885ads.dts
>> +++ b/arch/powerpc/boot/dts/mpc885ads.dts
>> @@ -213,6 +213,15 @@
>> fsl,cpm-command = <0080>;
>> linux,network-index = <2>;
>> };
>> +
>> + i2c@860 {
>> + device_type = "i2c";
>>
>
> No device_type.
>
Why? Documentation/powerpc/booting-without-of.txt says for I2C interfaces
device_type is required and should be "i2c". Is this no longer true?
> Should be fsl,cpm-i2c. Is cpm2 i2c the same? If not, it should be
> fsl,cpm1-i2c. It's probably best to specify it anyway, along with
> fsl,mpc885-i2c.
>
CPM2 i2c seems to be the same. However, i have no way to test this.
>> +#ifdef CONFIG_I2C_8XX
>> + setbits32(&mpc8xx_immr->im_cpm.cp_pbpar, 0x00000030);
>> + setbits32(&mpc8xx_immr->im_cpm.cp_pbdir, 0x00000030);
>> + setbits16(&mpc8xx_immr->im_cpm.cp_pbodr, 0x0030);
>> +#endif
>>
>
> Please add this to mpc885ads_pins, rather than poking the registers
> directly. The relevant lines are:
>
> {CPM_PORTB, 26, CPM_PIN_OUTPUT},
> {CPM_PORTB, 27, CPM_PIN_OUTPUT},
>
I noticed cpm1_set_pin32, but this function don't seem to set the
odr register. Will this be added? Then it would be:
{CPM_PORTB, 26, CPM_PIN_OUTPUT | CPM_PIN_OPENDRAIN},
{CPM_PORTB, 27, CPM_PIN_OUTPUT | CPM_PIN_OPENDRAIN},
>> + /* Select an arbitrary address. Just make sure it is unique.
>> + */
>> + out_8(&i2c->i2c_i2add, 0xfe);
>>
>
> It's a 7-bit address... and are you sure that 0x7e is unique? Does this
> driver even support slave operation?
>
It's in fact 0x7F << 1. The same value is used in the 2.4 driver and
in u-boot, as well. Slave operation is not supported.
> Why is an 8xx driver matching all i2c cpm (i.e. what about cpm2)?
>
With the suggested change to use fsl,cpm-command, the driver should
be able to use both cpm1 and cpm2. The operation and structs for i2c
are identical. The only difference might be the hack^wsupport for
relocation.
Thanks,
Jochen
Jochen Friedrich wrote:
>>> diff --git a/arch/powerpc/boot/dts/mpc885ads.dts
>>> b/arch/powerpc/boot/dts/mpc885ads.dts
>>> index 8848e63..a526c02 100644
>>> --- a/arch/powerpc/boot/dts/mpc885ads.dts
>>> +++ b/arch/powerpc/boot/dts/mpc885ads.dts
>>> @@ -213,6 +213,15 @@
>>> fsl,cpm-command = <0080>;
>>> linux,network-index = <2>;
>>> };
>>> +
>>> + i2c@860 {
>>> + device_type = "i2c";
>>>
>>
>> No device_type.
>>
>
> Why? Documentation/powerpc/booting-without-of.txt says for I2C interfaces
> device_type is required and should be "i2c". Is this no longer true?
booting-without-of.txt should be changed.
>> Should be fsl,cpm-i2c. Is cpm2 i2c the same? If not, it should be
>> fsl,cpm1-i2c. It's probably best to specify it anyway, along with
>> fsl,mpc885-i2c.
>
> CPM2 i2c seems to be the same. However, i have no way to test this.
OK, let's make the compatible "fsl,mpc885-i2c", "fsl,cpm1-i2c",
"fsl,cpm-i2c".
For now, match on the last one, but if any differences pop up, we have
the more specific ones.
> I noticed cpm1_set_pin32, but this function don't seem to set the
> odr register. Will this be added? Then it would be:
>
> {CPM_PORTB, 26, CPM_PIN_OUTPUT | CPM_PIN_OPENDRAIN},
> {CPM_PORTB, 27, CPM_PIN_OUTPUT | CPM_PIN_OPENDRAIN},
>
Ah, missed that -- there's opendrain support for port E, but I missed
that port B had it as well. Feel free to add it.
>> It's a 7-bit address... and are you sure that 0x7e is unique? Does this
>> driver even support slave operation?
>
> It's in fact 0x7F << 1.
Gah, I hate powerpc bit numbering, especially without the
numbered-as-if-64-bit hack. I specifically looked at the manual before
to see if it was shifted, saw "0-6", and concluded it wasn't. :-P
> The same value is used in the 2.4 driver and
> in u-boot, as well.
That doesn't mean that this isn't a good time to review what the code is
doing. :-)
> Slave operation is not supported.
If slave operation isn't supported, how is this value used?
>> Why is an 8xx driver matching all i2c cpm (i.e. what about cpm2)?
>
> With the suggested change to use fsl,cpm-command, the driver should
> be able to use both cpm1 and cpm2. The operation and structs for i2c
> are identical. The only difference might be the hack^wsupport for
> relocation.
OK. Would that allow it to become one driver, rather than a wrapper and
an algorithm?
-Scott
On Wed, 17 Oct 2007 21:20:37 +0200, Jochen Friedrich wrote:
> >> + /* Select an arbitrary address. Just make sure it is unique.
> >> + */
> >> + out_8(&i2c->i2c_i2add, 0xfe);
> >>
> >
> > It's a 7-bit address... and are you sure that 0x7e is unique? Does this
> > driver even support slave operation?
> >
>
> It's in fact 0x7F << 1. The same value is used in the 2.4 driver and
> in u-boot, as well. Slave operation is not supported.
I'm not sure what exactly you are doing here, but 0x7f isn't a valid
7-bit I2C address.
--
Jean Delvare
Hi Jean,
> On Wed, 17 Oct 2007 21:20:37 +0200, Jochen Friedrich wrote:
>>>> + /* Select an arbitrary address. Just make sure it is unique.
>>>> + */
>>>> + out_8(&i2c->i2c_i2add, 0xfe);
>>>>
>>> It's a 7-bit address... and are you sure that 0x7e is unique? Does this
>>> driver even support slave operation?
>>>
>> It's in fact 0x7F << 1. The same value is used in the 2.4 driver and
>> in u-boot, as well. Slave operation is not supported.
>
> I'm not sure what exactly you are doing here, but 0x7f isn't a valid
> 7-bit I2C address.
That's most probably the reason why it is used everywhere in the CPM world
(old driver, u-boot) ;-)
According to the documentation, the CPM enters a loopback mode as soon
as a write access to the I2C address written to i2c_i2add takes place.
This even happens if the CPM is set to master mode. So the only way to make
sure there are no limitations to the I2C addresses which can be used is to
use this invalid address. 0x00 can't be used either or no I2C broadcasts
would be possible.
>From the MPC-823 documentation:
16.13.3.1.3 I2C Loopback Configuration. Loopback on the I2C controller is a
special part of master mode operation with a device that does not contain
internal addresses. Refer to Figure 16-127 for more information. To begin a
loopback transmission, you must prepare a TX buffer descriptor with a data
buffer N+1 bytes long, where N is the number of data bytes to be written back
to the I2C controller. You must also prepare one or more RX buffer descriptors
to receive the N bytes of data. The first byte of the TX buffer descriptor
must contain the address of the MPC823 I2C device?s own address, which is in
the I2CADD register, followed by the write bit asserted (R/W = 0). The
remaining N bytes of the TX buffer descriptor contain the data to be sent and
received by the I2C controller. Next, set the R bit in the TX buffer descriptor
and the E bit in the RX buffer descriptor. Then set the W and L bits in the TX
buffer descriptor. Setting the L bit causes a stop condition to be issued after
this buffer is transmitted to conclude the operation. Set the I bit in the TX
and RX buffer descriptors to enable the transmission and reception status to be
updated in the I2CE register and to enable I2C transmit and receive interrupts
to the core. You must then set the STR bit in the I2COM register to initiate
the loopback operation.
Thanks,
Jochen