On 21 November 2011 21:58, Peter Chubb <pet...@gelato.unsw.edu.au> wrote:
> Hi Peter,
> Please find appended a patch containing initial support for the
> FreeScale i.MX31 and the KZM Arm11 evaluation board.
>
> The implementation was originally written by Hans Jang and Adam
> Clench of OK-Labs; I've updated it to the current qdev and memory
> region paradigms and implemented enough extra that Linux will boot
> on the patched QEMU using a ram disk.
>
> The i.MX 31 Serial controller is found in most of the i.MX SoCs;
> the AVIC and timer implementations can also be shared, albeit with
> fewer chips.
>
> Signed-off-by: Peter Chubb <peter.ch...@nicta.com.au>
> Signed-off-by: Hans Jang <hsj...@ok-labs.com>
> Signed-off-by: Adam Clench <ad...@ok-labs.com>
> ---
> Makefile.target | 1
> hw/imx_avic.c | 294 ++++++++++++++++++++++++++++++++++++++
> hw/imx_serial.c | 260 +++++++++++++++++++++++++++++++++
> hw/imx_timer.c | 430
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/kzm.c | 159 ++++++++++++++++++++
> 5 files changed, 1144 insertions(+)
This is a rather long patch. I think it would be easier to review
as a four patch series (1 patch per device plus 1 for the board model).
> create mode 100644 hw/imx_avic.c
> create mode 100644 hw/imx_serial.c
> create mode 100644 hw/imx_timer.c
> create mode 100644 hw/kzm.c
>
> Index: qemu-working/Makefile.target
> ===================================================================
> --- qemu-working.orig/Makefile.target 2011-11-22 08:40:56.380128155 +1100
> +++ qemu-working/Makefile.target 2011-11-22 08:42:26.288661513 +1100
> @@ -336,20 +336,21 @@ obj-sparc-y = sun4m.o lance.o tcx.o sun4
> obj-sparc-y += slavio_timer.o slavio_misc.o sparc32_dma.o
> obj-sparc-y += cs4231.o eccmemctl.o sbi.o sun4c_intctl.o leon3.o
>
> # GRLIB
> obj-sparc-y += grlib_gptimer.o grlib_irqmp.o grlib_apbuart.o
> endif
>
> obj-arm-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
> obj-arm-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o
> pl190.o
> obj-arm-y += versatile_pci.o
> +obj-arm-y += kzm.o imx_avic.o imx_serial.o imx_timer.o
> obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
> obj-arm-y += armv7m.o armv7m_nvic.o stellaris.o pl022.o stellaris_enet.o
> obj-arm-y += pl061.o
> obj-arm-y += arm-semi.o
> obj-arm-y += pxa2xx.o pxa2xx_pic.o pxa2xx_gpio.o pxa2xx_timer.o pxa2xx_dma.o
> obj-arm-y += pxa2xx_lcd.o pxa2xx_mmci.o pxa2xx_pcmcia.o pxa2xx_keypad.o
> obj-arm-y += gumstix.o
> obj-arm-y += zaurus.o ide/microdrive.o spitz.o tosa.o tc6393xb.o
> obj-arm-y += omap1.o omap_lcdc.o omap_dma.o omap_clk.o omap_mmc.o omap_i2c.o
> \
> omap_gpio.o omap_intc.o omap_uart.o
> Index: qemu-working/hw/imx_avic.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/imx_avic.c 2011-11-22 08:51:09.733239638 +1100
> @@ -0,0 +1,294 @@
> +/*
> + * IMX31 Vectored Interrupt Controller
> + *
> + * Note this is NOT the PL192 provided by ARM, but
> + * a custom implementation by FreeScale.
> + *
> + * Copyright (c) 2008 OKL
> + * Written by Hans
> + *
> + * This code is licenced under the GPL.
> + *
> + * TODO: implement vectors and priorities.
> + */
> +
> +#include "hw.h"
> +#include "sysbus.h"
> +#include <string.h> /* ffsll */
> +
> +#define DEBUG_INT 1
> +#undef DEBUG_INT /* comment out for debugging */
> +
> +#ifdef DEBUG_INT
> +#define DPRINTF(fmt, args...) \
> +do { printf("imx_int: " fmt , ##args); } while (0)
> +#else
> +#define DPRINTF(fmt, args...) do {} while (0)
> +#endif
> +
> +
> +#define IMX_INT_NUM_IRQS 64
> +
> +/* Interrupt Control Bits */
> +#define ABFLAG (1<<25)
> +#define ABFEN (1<<24)
> +#define NIDIS (1<<22) /* Normal Interrupt disable */
> +#define FIDIS (1<<21) /* Fast interrupt disable */
> +#define NIAD (1<<20) /* Normal Interrupt Arbiter Rise ARM level */
> +#define FIAD (1<<19) /* Fast Interrupt Arbiter Rise ARM level */
> +#define NM (1<<18) /* Normal interrupt mode */
> +
> +typedef struct {
> + SysBusDevice busdev;
> + MemoryRegion iomem;
> + uint64_t pending;
> + uint64_t enabled;
> + uint64_t is_fiq;
> + uint32_t intcntl;
> + uint32_t intmask;
> + qemu_irq irq;
> + qemu_irq fiq;
> + uint32_t prio[IMX_INT_NUM_IRQS/(32/4)]; /* Priorities are 4-bits each */
> +} imx_int_state;
> +
> +static inline int imx_int_prio(imx_int_state *s, int irq)
> +{
> + uint32_t word = irq / (32/4);
> + uint32_t part = irq % (32/4);
> + return 0xff & (s->prio[word] >> (4 * part));
If these are four bit fields (as suggested by the comment and the
size of the shift) why are we masking with 0xff rather than 0xf ?
Also can we avoid all this repetition of the (32/4) magic number?
> +static inline void imx_int_set_prio(imx_int_state *s, int irq, int prio)
> +{
> + uint32_t word = irq / (32/4);
> + uint32_t part = 4 * (irq % (32/4));
> + uint32_t mask = ~(0xff << part);
In the previous function we put the *4 in the shift; here we're
doing it in the calculation of part. Be consistent, please.
> + s->prio[word] &= mask;
> + s->prio[word] |= prio << part;
> +}
> +
> +/* Update interrupts. */
> +static void imx_int_update(imx_int_state *s)
> +{
> + int i;
> + uint64_t new = s->pending;
> + uint64_t flags;
> +
> + flags = new & s->enabled & s->is_fiq;
> + qemu_set_irq(s->fiq, !!flags);
> +
> + flags = new & s->enabled & ~s->is_fiq;
> + if (!flags || likely((s->intmask & 0x1f) == 0x1f)) {
Is the use of likely() here really justified?
> + qemu_set_irq(s->irq, !!flags);
> + return;
> + }
> + /* Take interrupt if prio lower than the value of intmask */
> +
> + for (i = 0; i < IMX_INT_NUM_IRQS; i++) {
> + if (flags & (1<<i)) {
> + if (imx_int_prio(s, i) > s->intmask) {
> + qemu_set_irq(s->irq, 1);
> + return;
> + }
> + }
> + }
> +
> +}
> +
> +static void imx_int_set_irq(void *opaque, int irq, int level)
> +{
> + imx_int_state *s = (imx_int_state *)opaque;
> +
> + if (level) {
> + s->pending |= (1ULL << irq);
> + } else {
> + s->pending &= ~(1ULL << irq);
> + }
> +
> + imx_int_update(s);
> +}
> +
> +
> +static uint64_t imx_int_read(void *opaque,
> + target_phys_addr_t offset, unsigned size)
> +{
> + imx_int_state *s = (imx_int_state *)opaque;
> +
> +
> + DPRINTF("read(offset = 0x%x)\n", offset >> 2);
> + switch (offset >> 2) {
> + case 0: /* INTCNTL */
> + return s->intcntl;
> +
> + case 1: /* Normal Interrupt Mask Register, NIMASK */
> + return s->intmask;
> +
> + case 2: /* Interrupt Enable Number Register, INTENNUM */
> + case 3: /* Interrupt Disable Number Register, INTDISNUM */
> + return 0;
> +
> + case 4: /* Interrupt Enabled Number Register High */
> + return s->enabled >> 32;
> + case 5: /* Interrupt Enabled Number Register Low */
> + return s->enabled & 0xffffffffULL;
> + case 6: /* Interrupt Type Register High */
> + return s->is_fiq >> 32;
> + case 7: /* Interrupt Type Register Low */
> + return s->is_fiq & 0xffffffffUll;
ULL
> + case 8: /* Normal Interrupt Priority Register 7 */
> + case 9: /* Normal Interrupt Priority Register 6 */
> + case 10:/* Normal Interrupt Priority Register 5 */
> + case 11:/* Normal Interrupt Priority Register 4 */
> + case 12:/* Normal Interrupt Priority Register 3 */
> + case 13:/* Normal Interrupt Priority Register 2 */
> + case 14:/* Normal Interrupt Priority Register 1 */
> + case 15:/* Normal Interrupt Priority Register 0 */
> + return s->prio[15-(offset>>2)];
> +
> + case 16: /* Normal interrupt vector and status register */
> + {
> + uint64_t flags = s->pending & s->enabled & ~s->is_fiq;
> + int i = ffsll(flags);
> + if (i) {
> + imx_int_set_irq(opaque, i-1, 0);
> + return (i-1) << 16;
> + }
> + return 0xFFFF<<16;
> + }
> + case 17:/* Fast Interrupt vector and status register */
> + {
> + uint64_t flags = s->pending & s->enabled & s->is_fiq;
> + int i = ffsll(flags);
> + if (i) {
> + imx_int_set_irq(opaque, i-1, 0);
> + return (i-1) << 16;
> + }
> + return 0xFFFF<<16;
> + }
> + case 18:/* Interrupt source register high */
> + return s->pending >> 32;
> + case 19:/* Interrupt source register low */
> + return s->pending & 0xFFFFFFFFULL;
> + case 20:/* Interrupt Force Register high */
> + case 21:/* Interrupt Force Register low */
> + return 0;
> + case 22:/* Normal Interrupt Pending Register High */
> + return (s->pending & s->enabled & ~s->is_fiq) >> 32;
> + case 23:/* Normal Interrupt Pending Register Low */
> + return (s->pending & s->enabled & ~s->is_fiq) & 0XFFFFFFFFULL;
0x
> + case 24: /* Fast Interrupt Pending Register High */
> + return (s->pending & s->enabled & s->is_fiq) >> 32;
> + case 25: /* Fast Interrupt Pending Register Low */
> + return (s->pending & s->enabled & s->is_fiq) & 0XFFFFFFFFULL;
> + case 0x40: /* AVIC vector 0, use for WFI WAR */
> + return 0x4;
> + default:
> + printf("imx_int_read: Bad offset 0x%x\n", (int)offset);
> + return 0;
> + }
> +}
> +
> +static void imx_int_write(void *opaque, target_phys_addr_t offset,
> + uint64_t val, unsigned size)
> +{
> + imx_int_state *s = (imx_int_state *)opaque;
> +
> + /* Vector Registers not yet supported */
> + if (offset >= 0x100 && offset <= 0x2fc) {
> + DPRINTF("imx_int_write to vector register %d\n",
> + (offset - 0x100)>>2);
> + return;
> + }
> +
> + DPRINTF("imx_int_write(0x%x) = %x\n",
> + (unsigned int)offset>>2, (unsigned int)val);
> + switch (offset >> 2) {
> + case 0: /* Interrupt Control Register, INTCNTL */
> + s->intcntl = val;
> + break;
> + case 1: /* Normal Interrupt Mask Register, NIMASK */
> + s->intmask = val;
> + break;
> + case 2: /* Interrupt Enable Number Register, INTENNUM */
> + DPRINTF("enable(%d)\n", (int)val);
> + s->enabled |= (1ULL << val);
> + break;
> + case 3: /* Interrupt Disable Number Register, INTDISNUM */
> + s->enabled &= ~(1ULL << val);
> + DPRINTF("disabled(%d)\n", (int)val);
> + break;
> + case 4: /* Interrupt Enable Number Register High */
> + s->enabled = (s->enabled & 0xffffffffULL) | (val << 32);
> + break;
> + case 5: /* Interrupt Enable Number Register Low */
> + s->enabled = (s->enabled & 0xffffffff00000000ULL) | val;
> + break;
> + case 6: /* Interrypt Type Register High */
Interrupt
> + s->is_fiq = (s->is_fiq & 0xffffffffULL) | (val << 32);
> + break;
> + case 7: /* Interrupt Type Register Low */
> + s->is_fiq = (s->is_fiq & 0xffffffff00000000ULL) | val;
> + break;
> + case 8: /* Normal Interrupt Priority Register 7 */
> + case 9: /* Normal Interrupt Priority Register 6 */
> + case 10:/* Normal Interrupt Priority Register 5 */
> + case 11:/* Normal Interrupt Priority Register 4 */
> + case 12:/* Normal Interrupt Priority Register 3 */
> + case 13:/* Normal Interrupt Priority Register 2 */
> + case 14:/* Normal Interrupt Priority Register 1 */
> + case 15:/* Normal Interrupt Priority Register 0 */
> + s->prio[15-(offset>>2)] = val;
> + return;
> + /* Read-only registers, writes ignored */
> + case 16:/* Normal Interrupt Vector and Status register */
> + case 17:/* Fast Interrupt vector and status register */
> + case 18:/* Interrupt source register high */
> + case 19:/* Interrupt source register low */
> + return;
> + case 20:/* Interrupt Force Register high */
> + s->pending = (s->pending & 0xffffffffULL) | (val << 32);
> + break;
> + case 21:/* Interrupt Force Register low */
> + s->pending = (s->pending & 0xffffffff00000000ULL) | val;
> + break;
> + case 22:/* Normal Interrupt Pending Register High */
> + case 23:/* Normal Interrupt Pending Register Low */
> + case 24: /* Fast Interrupt Pending Register High */
> + case 25: /* Fast Interrupt Pending Register Low */
> + return;
> + default:
> + hw_error("imx_int_write: Bad offset %x\n", (int)offset);
Don't hw_error() for guest bad behaviour.
> + }
> + imx_int_update(s);
> +}
> +
> +static const MemoryRegionOps imx_int_ops = {
> + .read = imx_int_read,
> + .write = imx_int_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int imx_int_init(SysBusDevice *dev)
> +{
> + imx_int_state *s = FROM_SYSBUS(imx_int_state, dev);;
> +
> + memory_region_init_io(&s->iomem, &imx_int_ops, s, "imx_int", 0x1000);
> + sysbus_init_mmio_region(dev, &s->iomem);
> +
> + qdev_init_gpio_in(&dev->qdev, imx_int_set_irq, IMX_INT_NUM_IRQS);
> + sysbus_init_irq(dev, &s->irq);
> + sysbus_init_irq(dev, &s->fiq);
> +
> + s->intmask = 0x1f;
> + s->enabled = 0ULL;
The code setting member fields should be in a reset function.
Can't you just say "s->enabled = 0;" ?
> + return 0;
> +}
> +
> +static void imx_int_register_devices(void)
> +{
> + sysbus_register_dev("imx_int", sizeof(imx_int_state),
> + imx_int_init);
Use sysbus_register_withprop(). Provide a VMState so you get save/restore.
> +}
> +
> +device_init(imx_int_register_devices);
This macro doesn't need a trailing semicolon and mostly we don't
seem to give it one.
> +
> Index: qemu-working/hw/imx_serial.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/imx_serial.c 2011-11-22 08:49:36.084276219 +1100
> @@ -0,0 +1,260 @@
> +/*
> + * IMX31 UARTS
> + *
> + * Copyright (c) 2008 OKL
> + * Written by Hans
> + *
> + * This code is licenced under the GPL.
> + * This is a `bare-bones' implementation of the IMX series serial ports.
> + * TODO:
> + * -- implement FIFOs. The real hardware has 32 word transmit
> + * and receive FIFOs
> + * -- implement DMA
> + * -- implement BAUD-rate and modem lines, for when the backend
> + * is a real serial device.
> + */
> +
> +#include "hw.h"
> +#include "sysbus.h"
> +#include "qemu-char.h"
> +
> +#define DEBUG_SERIAL 1
> +#undef DEBUG_SERIAL /* comment out for debugging */
> +
> +#ifdef DEBUG_SERIAL
> +#define DPRINTF(fmt, args...) \
> +do { printf("imx_serial: " fmt , ##args); } while (0)
> +#else
> +#define DPRINTF(fmt, args...) do {} while (0)
> +#endif
> +
> +typedef struct {
> + SysBusDevice busdev;
> + MemoryRegion iomem;
> + int32_t readbuff;
> +
> + uint32_t usr1;
> + uint32_t usr2;
> + uint32_t ucr1;
> + uint32_t uts1;
> +
> + uint32_t ubrm;
> + uint32_t ubrc;
> +
> + qemu_irq irq;
> + CharDriverState *chr;
> +} imx_state;
> +
> +#define URXD_CHARRDY (1<<15) /* character read is valid */
> +
> +#define USR1_TRDY (1<<13) /* Xmitter ready */
> +#define USR1_RRDY (1<<9) /* receiver ready */
> +
> +#define USR2_TXFE (1<<14) /* Transmit FIFO empty */
> +#define USR2_RDR (1<<0) /* Receiove data ready */
Receive
> +#define USR2_TXDC (1<<3) /* Transmission complete */
> +
> +#define UCR1_UARTEN (1<<0)
> +#define UCR1_RRDYEN (1<<9)
> +#define UCR1_TRDYEN (1<<13)
> +#define UCR1_TXMPTYEN (1<<6)
> +
> +#define UTS1_TXEMPTY (1<<6)
> +#define UTS1_RXEMPTY (1<<5)
> +#define UTS1_TXFULL (1<<4)
> +#define UTS1_RXFULL (1<<3)
> +
> +static void imx_update(imx_state *s)
> +{
> + uint32_t flags;
> +
> + flags = ((s->usr1 & s->ucr1)) & (USR1_TRDY|USR1_RRDY);
> + if (0 == (s->ucr1 & UCR1_TXMPTYEN)) {
We don't use the 0 == foo style in qemu.
> + flags &= ~USR1_TRDY;
> + }
> +
> + qemu_set_irq(s->irq, !!flags);
> +}
> +
> +static uint64_t imx_serial_read(void *opaque, target_phys_addr_t offset,
> + unsigned size)
> +{
> + imx_state *s = (imx_state *)opaque;
> + uint32_t c;
> +
> + DPRINTF("read(offset=%x)\n", offset >> 2);
> + switch (offset >> 2) {
> + case 0x0: /* URXD */
> + c = s->readbuff;
> + s->usr1 &= ~USR1_RRDY;
> + s->usr2 &= ~USR2_RDR;
> + s->uts1 |= UTS1_RXEMPTY;
> + imx_update(s);
> + qemu_chr_accept_input(s->chr);
> + return c | URXD_CHARRDY;
> +
> + case 0x20: /* UCR1 */
> + return s->ucr1;
> +
> + case 0x21: /* UCR2 */
> + return 1; /* reset complete */
> +
> + case 0x25: /* USR1 */
> + imx_update(s);
> + return s->usr1;
> +
> + case 0x26: /* USR2 */
> + imx_update(s);
> + return s->usr2;
> +
> +
> + case 0x2A: /* BRM Modulator */
> + return s->ubrm;
> +
> + case 0x2B: /* Baud Rate Count */
> + return s->ubrc;
> +
> + case 0x2d: /* UTS1 */
> + return s->uts1;
> +
> +
> + case 0x22: /* UCR3 */
> + case 0x23: /* UCR4 */
> + case 0x24: /* UFCR */
> + case 0x29: /* BRM Incremental */
> + return 0x0; /* TODO */
> +
> + default:
> + hw_error("imx_serial_read: bad offset: 0x%x\n", (int)offset);
> + /* Keep gcc happy: notreached */
> + return 0;
Don't hw_error here.
> + }
> +}
> +
> +
> +static void imx_serial_write(void *opaque, target_phys_addr_t offset,
> + uint64_t value, unsigned size)
> +{
> + imx_state *s = (imx_state *)opaque;
> + unsigned char ch;
> +
> + DPRINTF("write(offset=%x, value = %x)\n", offset >> 2, (unsigned
> int)value);
> + switch (offset >> 2) {
> + case 0x10: /* UTXD */
> + ch = value;
> + if (s->chr) {
> + qemu_chr_fe_write(s->chr, &ch, 1);
> + }
> + s->usr1 &= ~USR1_TRDY;
> + imx_update(s);
> + s->usr1 |= USR1_TRDY;
> + imx_update(s);
> +
> + break;
> +
> + case 0x20: /* UCR1 */
> + s->ucr1 = value;
> + DPRINTF("write(ucr1=%x)\n", (unsigned int)value);
> + imx_update(s);
> + break;
> +
> + case 0x26: /* USR2 */
> + /*
> + * Writing 1 to some bits clears them; all other
> + * values are ignored
> + */
> + value &= (1<<15)|(1<<13)|(1<<12)|(1<<11)|(1<<10)|
> + (1<<8)|(1<<7)|(1<<6)|(1<<4)|(1<<2)|(1<<1);
> + s->usr2 &= ~value;
> + break;
> +
> + /* Linux expects to see what it writes here. */
> + /* We don't currently alter the baud rate */
> + case 0x29: /* UBIR */
> + s->ubrc = value;
> + break;
> +
> + case 0x2a: /* UBRM */
> + s->ubrm = value;
> + break;
> +
> + case 0x21: /* UCR2 */
> + case 0x2d: /* UTS1 */
> + case 0x22: /* UCR3 */
> + case 0x23: /* UCR4 */
> + case 0x24: /* UFCR */
> + case 0x25: /* USR1 */
> + case 0x2c: /* BIPR1 */
> + /* TODO */
> + break;
> +
> + default:
> + hw_error("imx_serial_write: Bad offset 0x%x\n", (int)offset);
> + }
> +}
> +
> +static int imx_can_receive(void *opaque)
> +{
> + imx_state *s = (imx_state *)opaque;
> + return !(s->usr1 & USR1_RRDY);
> +}
> +
> +static void imx_put_data(void *opaque, uint32_t value)
> +{
> + imx_state *s = (imx_state *)opaque;
> +
> + s->usr1 |= USR1_RRDY;
> + s->usr2 |= USR2_RDR;
> + s->uts1 &= ~UTS1_RXEMPTY;
> + s->readbuff = value;
> + imx_update(s);
> +}
> +
> +static void imx_receive(void *opaque, const uint8_t *buf, int size)
> +{
> + imx_put_data(opaque, *buf);
> +}
> +
> +static void imx_event(void *opaque, int event)
> +{
> + if (event == CHR_EVENT_BREAK) {
> + imx_put_data(opaque, 0x400);
> + }
> +}
> +
> +static const struct MemoryRegionOps imx_serial_ops = {
> + .read = imx_serial_read,
> + .write = imx_serial_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int imx_serial_init(SysBusDevice *dev)
> +{
> + imx_state *s = FROM_SYSBUS(imx_state, dev);
> +
> + memory_region_init_io(&s->iomem, &imx_serial_ops, s, "imx-serial",
> 0x1000);
> + sysbus_init_mmio_region(dev, &s->iomem);
> + sysbus_init_irq(dev, &s->irq);
> + s->chr = qdev_init_chardev(&dev->qdev);
> +
> + s->usr1 = USR1_TRDY;
> + s->usr2 = USR2_TXFE | USR2_TXDC;
> + s->ucr1 = UCR1_TRDYEN | UCR1_RRDYEN | UCR1_UARTEN;
> + s->uts1 = UTS1_RXEMPTY;
> + s->readbuff = 0;
Initialisation of fields => in a reset function.
> + if (s->chr) {
> + qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive,
> + imx_event, s);
> + }
> + return 0;
> + /* ??? Save/restore. */
Implement save/restore :-)
> +}
> +
> +static void imx_serial_register_devices(void)
> +{
> + DPRINTF("imx_serial_register_devices\n");
> + sysbus_register_dev("imx_serial", sizeof(imx_state),
> + imx_serial_init);
> +}
> +
> +device_init(imx_serial_register_devices);
I'll come back and look at the timer and board later.
-- PMM
