From 228e84ad2a6c710591ba1d351fcbae635d17fd33 Mon Sep 17 00:00:00 2001 From: Thomas Cort Date: Mon, 12 Aug 2013 13:39:54 -0400 Subject: [PATCH] i2c: general clean-up Implement changes based on Kees comments on the code review: http://gerrit-minix.few.vu.nl/#/c/676/ Changes: - use spin API instead of micro_delay() for busy-wait loops. - use read16/write16/set16 from mmio.h to access I2C registers. - reduce the timeout for soft reset. - in read/write, don't mix variable declaration and initialization. - after transfer, wait for access ready instead of blindly delaying. - rename constants am335x, dm37xx to AM335X_I2C_BUS, DM37XX_I2C_BUS. - rename ADDRESS_MASK to MAX_I2C_SA_MASK for clairity. - rename omap_i2c_claim_bus() to omap_i2c_bus_is_free(). Change-Id: Ic221e6134e024ea0a6690f21cff208a728286948 --- drivers/i2c/arch/earm/omap_i2c.c | 222 +++++++++++---------- drivers/i2c/arch/earm/omap_i2c_registers.h | 2 +- 2 files changed, 113 insertions(+), 111 deletions(-) diff --git a/drivers/i2c/arch/earm/omap_i2c.c b/drivers/i2c/arch/earm/omap_i2c.c index 664a841bb..6925c963a 100644 --- a/drivers/i2c/arch/earm/omap_i2c.c +++ b/drivers/i2c/arch/earm/omap_i2c.c @@ -12,6 +12,7 @@ #include #include #include +#include /* device headers */ #include @@ -29,8 +30,13 @@ /* local headers */ #include "omap_i2c.h" -/* defines the set of register */ - +/* + * defines the set of register + * + * Warning: always use the 16-bit variants of read/write/set from mmio.h + * to access these registers. The DM37XX TRM Section 17.6 warns that 32-bit + * accesses can corrupt the register contents. + */ typedef struct omap_i2c_registers { vir_bytes I2C_REVNB_LO; /* AM335X Only */ @@ -75,7 +81,7 @@ typedef struct omap_i2c_registers typedef struct omap_i2c_bus { enum bus_types - { am335x, dm37xx } bus_type; + { AM335X_I2C_BUS, DM37XX_I2C_BUS} bus_type; phys_bytes mr_base; phys_bytes mr_size; vir_bytes mapped_addr; @@ -154,15 +160,15 @@ static omap_i2c_regs_t dm37xx_i2c_regs = { /* Define the buses available on each chip */ static omap_i2c_bus_t am335x_i2c_buses[] = { - {am335x, AM335X_I2C0_BASE, AM335X_I2C0_SIZE, 0, &am335x_i2c_regs, + {AM335X_I2C_BUS, AM335X_I2C0_BASE, AM335X_I2C0_SIZE, 0, &am335x_i2c_regs, AM335X_FUNCTIONAL_CLOCK, AM335X_MODULE_CLOCK, BUS_SPEED_400KHz, AM335X_REV_MAJOR, AM335X_REV_MINOR, AM335X_I2C0_IRQ, 1, 1}, - {am335x, AM335X_I2C1_BASE, AM335X_I2C1_SIZE, 0, &am335x_i2c_regs, + {AM335X_I2C_BUS, AM335X_I2C1_BASE, AM335X_I2C1_SIZE, 0, &am335x_i2c_regs, AM335X_FUNCTIONAL_CLOCK, AM335X_MODULE_CLOCK, BUS_SPEED_100KHz, AM335X_REV_MAJOR, AM335X_REV_MINOR, AM335X_I2C1_IRQ, 2, 3}, - {am335x, AM335X_I2C2_BASE, AM335X_I2C2_SIZE, 0, &am335x_i2c_regs, + {AM335X_I2C_BUS, AM335X_I2C2_BASE, AM335X_I2C2_SIZE, 0, &am335x_i2c_regs, AM335X_FUNCTIONAL_CLOCK, AM335X_MODULE_CLOCK, BUS_SPEED_100KHz, AM335X_REV_MAJOR, AM335X_REV_MINOR, AM335X_I2C2_IRQ, 3, 3} @@ -171,15 +177,15 @@ static omap_i2c_bus_t am335x_i2c_buses[] = { #define AM335X_OMAP_NBUSES (sizeof(am335x_i2c_buses) / sizeof(omap_i2c_bus_t)) static omap_i2c_bus_t dm37xx_i2c_buses[] = { - {dm37xx, DM37XX_I2C0_BASE, DM37XX_I2C0_SIZE, 0, &dm37xx_i2c_regs, + {DM37XX_I2C_BUS, DM37XX_I2C0_BASE, DM37XX_I2C0_SIZE, 0, &dm37xx_i2c_regs, DM37XX_FUNCTIONAL_CLOCK, DM37XX_MODULE_CLOCK, BUS_SPEED_100KHz, DM37XX_REV_MAJOR, DM37XX_REV_MINOR, DM37XX_I2C0_IRQ, 1, 1}, - {dm37xx, DM37XX_I2C1_BASE, DM37XX_I2C1_SIZE, 0, &dm37xx_i2c_regs, + {DM37XX_I2C_BUS, DM37XX_I2C1_BASE, DM37XX_I2C1_SIZE, 0, &dm37xx_i2c_regs, DM37XX_FUNCTIONAL_CLOCK, DM37XX_MODULE_CLOCK, BUS_SPEED_100KHz, DM37XX_REV_MAJOR, DM37XX_REV_MINOR, DM37XX_I2C1_IRQ, 2, 2}, - {dm37xx, DM37XX_I2C2_BASE, DM37XX_I2C2_SIZE, 0, &dm37xx_i2c_regs, + {DM37XX_I2C_BUS, DM37XX_I2C2_BASE, DM37XX_I2C2_SIZE, 0, &dm37xx_i2c_regs, DM37XX_FUNCTIONAL_CLOCK, DM37XX_MODULE_CLOCK, BUS_SPEED_100KHz, DM37XX_REV_MAJOR, DM37XX_REV_MINOR, DM37XX_I2C2_IRQ, 3, 3} @@ -208,8 +214,7 @@ static int omap_i2c_process(minix_i2c_ioctl_exec_t * m); /* Bus Specific Code */ static void omap_i2c_flush(void); static uint16_t omap_i2c_poll(uint16_t mask); -static int omap_i2c_claim_bus(void); -static int omap_i2c_release_bus(void); +static int omap_i2c_bus_is_free(void); static int omap_i2c_soft_reset(void); static void omap_i2c_bus_init(void); #if 0 @@ -224,13 +229,6 @@ static int omap_i2c_read(i2c_addr_t addr, uint8_t * buf, size_t buflen, static int omap_i2c_write(i2c_addr_t addr, const uint8_t * buf, size_t buflen, int dostop); -/* Helpers for register access */ - -#define reg_read(a) (*(volatile uint16_t *)(omap_i2c_bus->mapped_addr + a)) -#define reg_write(a,v) (*(volatile uint16_t *)(omap_i2c_bus->mapped_addr + a) = (v)) -#define reg_set_bit(a,v) reg_write((a), reg_read((a)) | (1<regs->I2C_DATA); + (void) read16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_DATA); /* clear the read ready flag */ omap_i2c_write_status(1 << RRDY); - micro_delay(250); - } else { break; /* buffer drained */ } @@ -324,55 +318,44 @@ omap_i2c_flush(void) static uint16_t omap_i2c_poll(uint16_t mask) { - int tries; + spin_t spin; uint16_t status; - for (tries = 0; tries < 1000; tries++) { - + /* poll for up to 250 ms */ + spin_init(&spin, 250000); + do { status = omap_i2c_read_status(); if ((status & mask) != 0) { /* any bits in mask set */ return status; } - micro_delay(250); /* else wait and try again */ - } + } while (spin_check(&spin)); return status; /* timeout reached, abort */ } /* - * Poll Bus Busy Flag until the bus becomes free (return 0) or the timeout - * expires (return -1). + * Poll Bus Busy Flag until the bus becomes free (return 1) or the timeout + * expires (return 0). */ static int -omap_i2c_claim_bus(void) +omap_i2c_bus_is_free(void) { - int tries; + spin_t spin; uint16_t status; - for (tries = 0; tries < 1000; tries++) { + /* wait for up to 1 second for the bus to become free */ + spin_init(&spin, 1000000); + do { status = omap_i2c_read_status(); if ((status & (1 << BB)) == 0) { - return 0; /* bus is free */ + return 1; /* bus is free */ } - micro_delay(1000); - } - - return -1; /* timeout expired */ -} - -/* - * Clean up after a transfer - */ -static int -omap_i2c_release_bus(void) -{ - omap_i2c_write_status(0x7fff); - micro_delay(50000); + } while (spin_check(&spin)); - return 0; + return 0; /* timeout expired */ } static void @@ -380,14 +363,14 @@ omap_i2c_clkconf(int i2c_bus_id) { clkconf_init(); - if (omap_i2c_bus->bus_type == dm37xx) { + if (omap_i2c_bus->bus_type == DM37XX_I2C_BUS) { clkconf_set(CM_ICLKEN1_CORE, BIT((15 + i2c_bus_id)), 0xffffffff); clkconf_set(CM_FCLKEN1_CORE, BIT((15 + i2c_bus_id)), 0xffffffff); - } else if (omap_i2c_bus->bus_type == am335x) { + } else if (omap_i2c_bus->bus_type == AM335X_I2C_BUS) { switch (i2c_bus_id) { case 0: @@ -417,7 +400,7 @@ omap_i2c_padconf(int i2c_bus_id) padconf_init(); - if (omap_i2c_bus->bus_type == am335x) { + if (omap_i2c_bus->bus_type == AM335X_I2C_BUS) { /* use the options suggested in starterware driver */ pinopts = @@ -466,27 +449,27 @@ omap_i2c_padconf(int i2c_bus_id) static int omap_i2c_soft_reset(void) { - int tries; + spin_t spin; /* Disable to do soft reset */ - reg_write(omap_i2c_bus->regs->I2C_CON, 0); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CON, 0); micro_delay(50000); /* Do a soft reset */ - reg_write(omap_i2c_bus->regs->I2C_SYSC, (1 << SRST)); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_SYSC, (1 << SRST)); /* Have to temporarily enable I2C to read RDONE */ - reg_set_bit(omap_i2c_bus->regs->I2C_CON, I2C_EN); + set16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CON, (1<regs->I2C_SYSS) & (1 << RDONE)) { + /* wait up to 3 seconds for reset to complete */ + spin_init(&spin, 3000000); + do { + if (read16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_SYSS) & (1 << RDONE)) { return OK; } - micro_delay(1000); - } + + } while (spin_check(&spin)); log_warn(&log, "Tried soft reset, but bus never came back.\n"); return EIO; @@ -532,10 +515,10 @@ omap_i2c_intr_enable(void) intmask |= (1 << NACK); intmask |= (1 << AL); - if (omap_i2c_bus->bus_type == am335x) { - reg_write(omap_i2c_bus->regs->I2C_IRQENABLE_SET, intmask); - } else if (omap_i2c_bus->bus_type == dm37xx) { - reg_write(omap_i2c_bus->regs->I2C_IE, intmask); + if (omap_i2c_bus->bus_type == AM335X_I2C_BUS) { + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_IRQENABLE_SET, intmask); + } else if (omap_i2c_bus->bus_type == DM37XX_I2C_BUS) { + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_IE, intmask); } else { log_warn(&log, "Don't know how to enable interrupts.\n"); } @@ -546,39 +529,39 @@ omap_i2c_bus_init(void) { /* Ensure i2c module is disabled before setting prescalar & bus speed */ - reg_write(omap_i2c_bus->regs->I2C_CON, 0); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CON, 0); micro_delay(50000); /* Disable autoidle */ - reg_clear_bit(omap_i2c_bus->regs->I2C_SYSC, AUTOIDLE); + set16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_SYSC, (1<regs->I2C_PSC, + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_PSC, ((omap_i2c_bus->functional_clock / omap_i2c_bus->module_clock) - 1)); /* Set the bus speed */ - reg_write(omap_i2c_bus->regs->I2C_SCLL, + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_SCLL, ((omap_i2c_bus->module_clock / (2 * omap_i2c_bus->bus_speed)) - 7)); - reg_write(omap_i2c_bus->regs->I2C_SCLH, + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_SCLH, ((omap_i2c_bus->module_clock / (2 * omap_i2c_bus->bus_speed)) - 5)); /* Set own I2C address */ - if (omap_i2c_bus->bus_type == am335x) { - reg_write(omap_i2c_bus->regs->I2C_OA, I2C_OWN_ADDRESS); - } else if (omap_i2c_bus->bus_type == dm37xx) { - reg_write(omap_i2c_bus->regs->I2C_OA0, I2C_OWN_ADDRESS); + if (omap_i2c_bus->bus_type == AM335X_I2C_BUS) { + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_OA, I2C_OWN_ADDRESS); + } else if (omap_i2c_bus->bus_type == DM37XX_I2C_BUS) { + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_OA0, I2C_OWN_ADDRESS); } else { log_warn(&log, "Don't know how to set own address.\n"); } /* Set TX/RX Threshold to 1 and disable I2C DMA */ - reg_write(omap_i2c_bus->regs->I2C_BUF, 0x0000); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_BUF, 0x0000); /* Bring the i2c module out of reset */ - reg_set_bit(omap_i2c_bus->regs->I2C_CON, I2C_EN); + set16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CON, (1<bus_type == am335x) { + if (omap_i2c_bus->bus_type == AM335X_I2C_BUS) { /* TRM says to use RAW for polling for events */ - status = reg_read(omap_i2c_bus->regs->I2C_IRQSTATUS_RAW); - } else if (omap_i2c_bus->bus_type == dm37xx) { - status = reg_read(omap_i2c_bus->regs->I2C_STAT); + status = read16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_IRQSTATUS_RAW); + } else if (omap_i2c_bus->bus_type == DM37XX_I2C_BUS) { + status = read16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_STAT); } else { log_warn(&log, "Don't know how to read i2c bus status.\n"); } @@ -607,11 +590,11 @@ omap_i2c_read_status(void) static void omap_i2c_write_status(uint16_t mask) { - if (omap_i2c_bus->bus_type == am335x) { + if (omap_i2c_bus->bus_type == AM335X_I2C_BUS) { /* write 1's to IRQSTATUS (not RAW) to clear the bits */ - reg_write(omap_i2c_bus->regs->I2C_IRQSTATUS, mask); - } else if (omap_i2c_bus->bus_type == dm37xx) { - reg_write(omap_i2c_bus->regs->I2C_STAT, mask); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_IRQSTATUS, mask); + } else if (omap_i2c_bus->bus_type == DM37XX_I2C_BUS) { + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_STAT, mask); } else { log_warn(&log, "Don't know how to clear i2c bus status.\n"); } @@ -621,27 +604,30 @@ static int omap_i2c_read(i2c_addr_t addr, uint8_t * buf, size_t buflen, int dostop) { int r, i; - uint16_t conopts = 0; - uint16_t pollmask = 0; - uint16_t errmask = 0; + uint16_t conopts; + uint16_t pollmask; + uint16_t errmask; /* Set address of slave device */ - addr &= ADDRESS_MASK; /* sanitize address (10-bit max) */ + conopts = 0; + addr &= MAX_I2C_SA_MASK; /* sanitize address (10-bit max) */ if (addr > 0x7f) { /* 10-bit extended address in use, need to set XSA */ conopts |= (1 << XSA); } + errmask = 0; errmask |= (1 << ROVR); errmask |= (1 << AERR); errmask |= (1 << NACK); errmask |= (1 << AL); + pollmask = 0; pollmask |= (1 << RRDY); /* Set bytes to read and slave address */ - reg_write(omap_i2c_bus->regs->I2C_CNT, buflen); - reg_write(omap_i2c_bus->regs->I2C_SA, addr); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CNT, buflen); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_SA, addr); /* Set control register */ conopts |= (1 << I2C_EN); /* enabled */ @@ -652,7 +638,7 @@ omap_i2c_read(i2c_addr_t addr, uint8_t * buf, size_t buflen, int dostop) conopts |= (1 << STP); /* stop condition */ } - reg_write(omap_i2c_bus->regs->I2C_CON, conopts); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CON, conopts); for (i = 0; i < buflen; i++) { /* Data to read? */ @@ -669,7 +655,7 @@ omap_i2c_read(i2c_addr_t addr, uint8_t * buf, size_t buflen, int dostop) } /* read a byte */ - buf[i] = reg_read(omap_i2c_bus->regs->I2C_DATA) & 0xff; + buf[i] = read16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_DATA) & 0xff; /* clear the read ready flag */ omap_i2c_write_status(pollmask); @@ -681,8 +667,14 @@ omap_i2c_read(i2c_addr_t addr, uint8_t * buf, size_t buflen, int dostop) return EIO; } + /* Wait for operation to complete */ + pollmask = (1< 0x7f) { /* 10-bit extended address in use, need to set XSA */ conopts |= (1 << XSA); } + pollmask = 0; pollmask |= (1 << XRDY); + errmask = 0; errmask |= (1 << ROVR); errmask |= (1 << AERR); errmask |= (1 << NACK); errmask |= (1 << AL); - reg_write(omap_i2c_bus->regs->I2C_CNT, buflen); - reg_write(omap_i2c_bus->regs->I2C_SA, addr); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CNT, buflen); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_SA, addr); /* Set control register */ conopts |= (1 << I2C_EN); /* enabled */ @@ -723,7 +718,7 @@ omap_i2c_write(i2c_addr_t addr, const uint8_t * buf, size_t buflen, int dostop) } omap_i2c_write_status(0x7fff); - reg_write(omap_i2c_bus->regs->I2C_CON, conopts); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_CON, conopts); for (i = 0; i < buflen; i++) { @@ -737,7 +732,7 @@ omap_i2c_write(i2c_addr_t addr, const uint8_t * buf, size_t buflen, int dostop) return EBUSY; } - reg_write(omap_i2c_bus->regs->I2C_DATA, buf[i]); + write16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_DATA, buf[i]); /* clear the write ready flag */ omap_i2c_write_status(pollmask); @@ -745,11 +740,18 @@ omap_i2c_write(i2c_addr_t addr, const uint8_t * buf, size_t buflen, int dostop) r = omap_i2c_read_status(); if ((r & (1 << NACK)) != 0) { + log_warn(&log, "NACK\n"); return EIO; } + /* Wait for operation to complete */ + pollmask = (1<bus_type == am335x) { + if (omap_i2c_bus->bus_type == AM335X_I2C_BUS) { /* I2C_REVLO revision: major (bits 10-8), minor (bits 5-0) */ - i2c_rev = reg_read(omap_i2c_bus->regs->I2C_REVNB_LO); + i2c_rev = read16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_REVNB_LO); major = (i2c_rev >> 8) & 0x07; minor = i2c_rev & 0x3f; - } else if (omap_i2c_bus->bus_type == dm37xx) { + } else if (omap_i2c_bus->bus_type == DM37XX_I2C_BUS) { /* I2C_REV revision: major (bits 7-4), minor (bits 3-0) */ - i2c_rev = reg_read(omap_i2c_bus->regs->I2C_REV); + i2c_rev = read16(omap_i2c_bus->mapped_addr + omap_i2c_bus->regs->I2C_REV); major = (i2c_rev >> 4) & 0x0f; minor = i2c_rev & 0x0f; } else { diff --git a/drivers/i2c/arch/earm/omap_i2c_registers.h b/drivers/i2c/arch/earm/omap_i2c_registers.h index ce6932ca8..6563ca040 100644 --- a/drivers/i2c/arch/earm/omap_i2c_registers.h +++ b/drivers/i2c/arch/earm/omap_i2c_registers.h @@ -115,7 +115,7 @@ /* Masks */ -#define ADDRESS_MASK (0x3ff) /* Highest 10 bit address -- 9..0 */ +#define MAX_I2C_SA_MASK (0x3ff) /* Highest 10 bit address -- 9..0 */ /* Bit Offsets within Registers (only those used are listed) */ -- 2.44.0