| Message ID | ea98b8b7dd1f98f61577aa6fbc6cfca620907ed6.1367676178.git.jcd@tribudubois.net |
|---|---|
| State | New |
| Headers | show |
Hi JC, Thanks for actioning the comments. On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: > The slave mode is not implemented. > > Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net> > --- As a general rule you need to indicate changes between the last version and this version (changlog). My personal preference is to do it here - below the line on individual patch emails, although others do their changlogs in cover letters for entire series. Either system is acceptable. > default-configs/arm-softmmu.mak | 2 + > hw/i2c/Makefile.objs | 1 + > hw/i2c/imx_i2c.c | 383 ++++++++++++++++++++++++++++++++++++++++ > include/hw/arm/imx.h | 1 + > 4 files changed, 387 insertions(+) > create mode 100644 hw/i2c/imx_i2c.c > > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak > index b3a0207..a20f112 100644 > --- a/default-configs/arm-softmmu.mak > +++ b/default-configs/arm-softmmu.mak > @@ -81,3 +81,5 @@ CONFIG_VERSATILE_PCI=y > CONFIG_VERSATILE_I2C=y > > CONFIG_SDHCI=y > + > +CONFIG_IMX_I2C=y > diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs > index 648278e..d27bbaa 100644 > --- a/hw/i2c/Makefile.objs > +++ b/hw/i2c/Makefile.objs > @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o > common-obj-$(CONFIG_APM) += pm_smbus.o > common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o > common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o > +common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o > obj-$(CONFIG_OMAP) += omap_i2c.o > diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c > new file mode 100644 > index 0000000..5b0d046 > --- /dev/null > +++ b/hw/i2c/imx_i2c.c > @@ -0,0 +1,383 @@ > +/* > + * i.MX I2C Bus Serial Interface Emulation > + * > + * Copyright (C) 2013 Jean-Christophe Dubois. > + * > + * 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, see <http://www.gnu.org/licenses/>. > + * > + */ > + > +#include "hw/sysbus.h" > +#include "hw/i2c/i2c.h" > + > +#ifndef IMX_I2C_DEBUG > +#define IMX_I2C_DEBUG 0 > +#endif > + > +#define TYPE_IMX_I2C "imx.i2c" > +#define IMX_I2C(obj) \ > + OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C) > + > +/* i.MX I2C memory map */ > +#define IMX_I2C_MEM_SIZE 0x14 > +#define IADR_ADDR 0x00 /* address register */ > +#define IFDR_ADDR 0x04 /* frequency divider register */ > +#define I2CR_ADDR 0x08 /* control register */ > +#define I2SR_ADDR 0x0c /* status register */ > +#define I2DR_ADDR 0x10 /* data register */ > + > +#define IADR_MASK 0xFE > +#define IADR_RESET 0 > + > +#define IFDR_MASK 0x3F > +#define IFDR_RESET 0 > + > +#define I2CR_IEN (1 << 7) > +#define I2CR_IIEN (1 << 6) > +#define I2CR_MSTA (1 << 5) > +#define I2CR_MTX (1 << 4) > +#define I2CR_TXAK (1 << 3) > +#define I2CR_RSTA (1 << 2) > +#define I2CR_MASK 0xFC > +#define I2CR_RESET 0 > + > +#define I2SR_ICF (1 << 7) > +#define I2SR_IAAF (1 << 6) > +#define I2SR_IBB (1 << 5) > +#define I2SR_IAL (1 << 4) > +#define I2SR_SRW (1 << 2) > +#define I2SR_IIF (1 << 1) > +#define I2SR_RXAK (1 << 0) > +#define I2SR_MASK 0xE9 > +#define I2SR_RESET 0x81 > + > +#define I2DR_MASK 0xFF > +#define I2DR_RESET 0 > + > +#define ADDR_RESET 0xFF00 > + > +#if IMX_I2C_DEBUG > +#define DPRINT(fmt, args...) \ > + do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0) > + > +static const char *imx_i2c_get_regname(unsigned offset) > +{ > + switch (offset) { > + case IADR_ADDR: > + return "IADR"; > + case IFDR_ADDR: > + return "IFDR"; > + case I2CR_ADDR: > + return "I2CR"; > + case I2SR_ADDR: > + return "I2SR"; > + case I2DR_ADDR: > + return "I2DR"; > + default: > + return "[?]"; > + } > +} > +#else > +#define DPRINT(fmt, args...) do { } while (0) > +#endif > + > +typedef struct imx_i2c_state { types should be in CamelCase IMXI2CState > + SysBusDevice parent_obj; > + MemoryRegion iomem; > + i2c_bus *bus; > + qemu_irq irq; > + > + uint16_t address; > + > + uint16_t iadr; > + uint16_t ifdr; > + uint16_t i2cr; > + uint16_t i2sr; > + uint16_t i2dr_read; > + uint16_t i2dr_write; > +} imx_i2c_state; > + > +static inline bool imx_i2c_is_enabled(imx_i2c_state *s) > +{ > + return s->i2cr & I2CR_IEN; > +} > + > +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s) > +{ > + return s->i2cr & I2CR_IIEN; > +} > + > +static inline bool imx_i2c_is_master(imx_i2c_state *s) > +{ > + return s->i2cr & I2CR_MSTA; > +} > + > +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s) > +{ > + return s->i2cr & I2CR_MTX; > +} > + > +static void imx_i2c_reset(DeviceState *dev) > +{ > + imx_i2c_state *s = IMX_I2C(dev); > + > + if (s->address != ADDR_RESET) { > + i2c_end_transfer(s->bus); > + } > + > + s->address = ADDR_RESET; > + s->iadr = IADR_RESET; > + s->ifdr = IFDR_RESET; > + s->i2cr = I2CR_RESET; > + s->i2sr = I2SR_RESET; > + s->i2dr_read = I2DR_RESET; > + s->i2dr_write = I2DR_RESET; > +} > + > +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s) > +{ > + /* > + * raise an interrupt if the device is enabled and it is configured > + * to generate some interrupts. > + */ > + if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) { > + s->i2sr |= I2SR_IIF; > + qemu_irq_raise(s->irq); > + } > +} > + > +static uint64_t imx_i2c_read(void *opaque, hwaddr offset, > + unsigned size) > +{ > + uint16_t value; > + imx_i2c_state *s = IMX_I2C(opaque); > + > + switch (offset) { > + case IADR_ADDR: > + value = s->iadr; > + break; > + case IFDR_ADDR: > + value = s->ifdr; > + break; > + case I2CR_ADDR: > + value = s->i2cr; > + break; > + case I2SR_ADDR: > + value = s->i2sr; > + break; > + case I2DR_ADDR: > + value = s->i2dr_read; > + > + if (imx_i2c_is_master(s)) { /* master mode */ > + int ret = 0xff; > + > + if (s->address == ADDR_RESET) { > + /* something is wrong as the address is not set */ > + DPRINT("Trying to read without specifying the slave address\n"); > + } else if (s->i2cr & I2CR_MTX) { > + DPRINT("Trying to read but MTX is set\n"); > + } else { > + /* get the next byte */ > + ret = i2c_recv(s->bus); > + > + if (ret >= 0) { > + imx_i2c_raise_interrupt(s); > + } else { > + DPRINT("read failed for device 0x%02x\n" s->address); > + ret = 0xff; > + } > + } > + > + s->i2dr_read = ret; > + } > + break; > + default: > + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset); > + break; > + } > + > + DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset) General question for the list, are we encouraging the use or PRIx in new code to remove the needs for casts from uint_XXt to unsigned in printfery? , > + (unsigned int)offset, value); > + > + return (uint64_t)value; > +} > + > +static void imx_i2c_write(void *opaque, hwaddr offset, > + uint64_t value, unsigned size) > +{ > + imx_i2c_state *s = IMX_I2C(opaque); > + > + DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset), > + (unsigned int)offset, (int)value); > + > + value &= 0xff; > + > + switch (offset) { > + case IADR_ADDR: > + s->iadr = value & IADR_MASK; > + /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */ > + break; > + case IFDR_ADDR: > + s->ifdr = value & IFDR_MASK; > + break; > + case I2CR_ADDR: > + if (imx_i2c_is_enabled(s) && ((value & I2CR_IEN) == 0)) { > + /* This is a soft reset. IADR is preserved during soft resets */ > + uint16_t iadr = s->iadr; > + imx_i2c_reset(&s->parent_obj.qdev); > + s->iadr = iadr; > + } else { /* normal write */ > + s->i2cr = value & I2CR_MASK; > + > + if (imx_i2c_is_master(s)) { /* master mode */ > + /* set the bus to busy */ > + s->i2sr |= I2SR_IBB; > + } else { /* slave mode */ > + /* bus is not busy anymore */ > + s->i2sr &= ~I2SR_IBB; > + > + /* > + *if we unset the master mode then it ends the ongoing > + * transfer if any > + */ > + if (s->address != ADDR_RESET) { > + i2c_end_transfer(s->bus); > + s->address = ADDR_RESET; > + } > + } > + > + if (s->i2cr & I2CR_RSTA) { /* Restart */ > + /* if this is a restart then it ends the ongoing transfer */ > + if (s->address != ADDR_RESET) { > + i2c_end_transfer(s->bus); > + s->address = ADDR_RESET; > + s->i2cr &= ~I2CR_RSTA; > + } > + } > + } > + break; > + case I2SR_ADDR: > + /* > + * if the user writes 0 to IIF then lower the interrupt and > + * reset the bit > + */ > + if ((s->i2sr & I2SR_IIF) && !(value & I2SR_IIF)) { > + s->i2sr &= ~I2SR_IIF; > + qemu_irq_lower(s->irq); > + } > + > + /* > + * if the user writes 0 to IAL, reset the bit > + */ > + if ((s->i2sr & I2SR_IAL) && !(value & I2SR_IAL)) { > + s->i2sr &= ~I2SR_IAL; > + } > + > + break; > + case I2DR_ADDR: > + /* if the device is not enabled, nothing to do */ > + if (!imx_i2c_is_enabled(s)) { > + break; > + } > + > + s->i2dr_write = value & I2DR_MASK; > + > + if (imx_i2c_is_master(s)) { /* master mode */ > + /* If this is the first write cycle then it is the slave addr */ > + if (s->address == ADDR_RESET) { > + if (i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1, You could use an extract macro. extract32 is generally considered better than shift and & logicvfor multi-bit fields. Ill have to look up if extract16 exists (im away from tree for this review) > + (int)s->i2dr_write & 0x01)) { > + /* if non zero is returned, the adress is not valid */ > + s->i2sr |= I2SR_RXAK; > + } else { > + s->address = s->i2dr_write; > + s->i2sr &= ~I2SR_RXAK; > + imx_i2c_raise_interrupt(s); > + } > + } else { /* This is a normal data write */ > + if (i2c_send(s->bus, s->i2dr_write)) { > + /* if the target return non zero then end the transfer */ > + s->i2sr |= I2SR_RXAK; > + s->address = ADDR_RESET; > + i2c_end_transfer(s->bus); > + } else { > + s->i2sr &= ~I2SR_RXAK; > + imx_i2c_raise_interrupt(s); > + } > + } > + } > + break; > + default: > + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset); > + break; > + } > +} > + > +static const MemoryRegionOps imx_i2c_ops = { > + .read = imx_i2c_read, > + .write = imx_i2c_write, > + .valid.min_access_size = 1, > + .valid.max_access_size = 2, > + .endianness = DEVICE_NATIVE_ENDIAN, > +}; > + > +static const VMStateDescription imx_i2c_vmstate = { > + .name = TYPE_IMX_I2C, > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT16(address, imx_i2c_state), > + VMSTATE_UINT16(iadr, imx_i2c_state), > + VMSTATE_UINT16(ifdr, imx_i2c_state), > + VMSTATE_UINT16(i2cr, imx_i2c_state), > + VMSTATE_UINT16(i2sr, imx_i2c_state), > + VMSTATE_UINT16(i2dr_read, imx_i2c_state), > + VMSTATE_UINT16(i2dr_write, imx_i2c_state), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static void imx_i2c_realize(DeviceState *dev, Error **errp) > +{ > + imx_i2c_state *s = IMX_I2C(dev); SysBusDevice *sbd = SYS_BUS_DEVICE(dev); to avoid the repeated casts below (and dereferencing of inline macros although that has another solution i've mentioned below). This rule is a little hot off the press and Andreas only just updated the QOMConventions page this week to clarify this better. > + > + memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C, > + IMX_I2C_MEM_SIZE); > + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); > + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); > + s->bus = i2c_init_bus(&SYS_BUS_DEVICE(dev)->qdev, "i2c"); The &foo->qdev is the old style dereferencing downcast which we are trying to eliminate. Instead this line should actually just be: s->bus = i2c_init_bus(DEVICE(dev), "i2c"); Consider the ->qdev (or parent_obj) field poisoned globally for all QOM objects. Regards, Peter > +} > + > +static void imx_i2c_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->vmsd = &imx_i2c_vmstate; > + dc->reset = imx_i2c_reset; > + dc->realize = imx_i2c_realize; > +} > + > +static const TypeInfo imx_i2c_type_info = { > + .name = TYPE_IMX_I2C, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(imx_i2c_state), > + .class_init = imx_i2c_class_init, > +}; > + > +static void imx_i2c_register_types(void) > +{ > + type_register_static(&imx_i2c_type_info); > +} > + > +type_init(imx_i2c_register_types) > diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h > index ea9e093..a875171 100644 > --- a/include/hw/arm/imx.h > +++ b/include/hw/arm/imx.h > @@ -30,5 +30,6 @@ void imx_timerg_create(const hwaddr addr, > qemu_irq irq, > DeviceState *ccm); > > +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq); > > #endif /* IMX_H */ > -- > 1.8.1.2 > >
On 05/05/2013 05:14 AM, Peter Crosthwaite wrote: > Hi JC, > > Thanks for actioning the comments. > > On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS > <jcd@tribudubois.net> wrote: >> The slave mode is not implemented. >> >> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net> >> --- > As a general rule you need to indicate changes between the last > version and this version (changlog). My personal preference is to do > it here - below the line on individual patch emails, although others > do their changlogs in cover letters for entire series. Either system > is acceptable. I'll do > >> default-configs/arm-softmmu.mak | 2 + >> hw/i2c/Makefile.objs | 1 + >> hw/i2c/imx_i2c.c | 383 ++++++++++++++++++++++++++++++++++++++++ >> include/hw/arm/imx.h | 1 + >> 4 files changed, 387 insertions(+) >> create mode 100644 hw/i2c/imx_i2c.c >> >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >> index b3a0207..a20f112 100644 >> --- a/default-configs/arm-softmmu.mak >> +++ b/default-configs/arm-softmmu.mak >> @@ -81,3 +81,5 @@ CONFIG_VERSATILE_PCI=y >> CONFIG_VERSATILE_I2C=y >> >> CONFIG_SDHCI=y >> + >> +CONFIG_IMX_I2C=y >> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs >> index 648278e..d27bbaa 100644 >> --- a/hw/i2c/Makefile.objs >> +++ b/hw/i2c/Makefile.objs >> @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o >> common-obj-$(CONFIG_APM) += pm_smbus.o >> common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o >> common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o >> +common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o >> obj-$(CONFIG_OMAP) += omap_i2c.o >> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >> new file mode 100644 >> index 0000000..5b0d046 >> --- /dev/null >> +++ b/hw/i2c/imx_i2c.c >> @@ -0,0 +1,383 @@ >> +/* >> + * i.MX I2C Bus Serial Interface Emulation >> + * >> + * Copyright (C) 2013 Jean-Christophe Dubois. >> + * >> + * 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, see <http://www.gnu.org/licenses/>. >> + * >> + */ >> + >> +#include "hw/sysbus.h" >> +#include "hw/i2c/i2c.h" >> + >> +#ifndef IMX_I2C_DEBUG >> +#define IMX_I2C_DEBUG 0 >> +#endif >> + >> +#define TYPE_IMX_I2C "imx.i2c" >> +#define IMX_I2C(obj) \ >> + OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C) >> + >> +/* i.MX I2C memory map */ >> +#define IMX_I2C_MEM_SIZE 0x14 >> +#define IADR_ADDR 0x00 /* address register */ >> +#define IFDR_ADDR 0x04 /* frequency divider register */ >> +#define I2CR_ADDR 0x08 /* control register */ >> +#define I2SR_ADDR 0x0c /* status register */ >> +#define I2DR_ADDR 0x10 /* data register */ >> + >> +#define IADR_MASK 0xFE >> +#define IADR_RESET 0 >> + >> +#define IFDR_MASK 0x3F >> +#define IFDR_RESET 0 >> + >> +#define I2CR_IEN (1 << 7) >> +#define I2CR_IIEN (1 << 6) >> +#define I2CR_MSTA (1 << 5) >> +#define I2CR_MTX (1 << 4) >> +#define I2CR_TXAK (1 << 3) >> +#define I2CR_RSTA (1 << 2) >> +#define I2CR_MASK 0xFC >> +#define I2CR_RESET 0 >> + >> +#define I2SR_ICF (1 << 7) >> +#define I2SR_IAAF (1 << 6) >> +#define I2SR_IBB (1 << 5) >> +#define I2SR_IAL (1 << 4) >> +#define I2SR_SRW (1 << 2) >> +#define I2SR_IIF (1 << 1) >> +#define I2SR_RXAK (1 << 0) >> +#define I2SR_MASK 0xE9 >> +#define I2SR_RESET 0x81 >> + >> +#define I2DR_MASK 0xFF >> +#define I2DR_RESET 0 >> + >> +#define ADDR_RESET 0xFF00 >> + >> +#if IMX_I2C_DEBUG >> +#define DPRINT(fmt, args...) \ >> + do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0) >> + >> +static const char *imx_i2c_get_regname(unsigned offset) >> +{ >> + switch (offset) { >> + case IADR_ADDR: >> + return "IADR"; >> + case IFDR_ADDR: >> + return "IFDR"; >> + case I2CR_ADDR: >> + return "I2CR"; >> + case I2SR_ADDR: >> + return "I2SR"; >> + case I2DR_ADDR: >> + return "I2DR"; >> + default: >> + return "[?]"; >> + } >> +} >> +#else >> +#define DPRINT(fmt, args...) do { } while (0) >> +#endif >> + >> +typedef struct imx_i2c_state { > types should be in CamelCase IMXI2CState OK > >> + SysBusDevice parent_obj; >> + MemoryRegion iomem; >> + i2c_bus *bus; >> + qemu_irq irq; >> + >> + uint16_t address; >> + >> + uint16_t iadr; >> + uint16_t ifdr; >> + uint16_t i2cr; >> + uint16_t i2sr; >> + uint16_t i2dr_read; >> + uint16_t i2dr_write; >> +} imx_i2c_state; >> + >> +static inline bool imx_i2c_is_enabled(imx_i2c_state *s) >> +{ >> + return s->i2cr & I2CR_IEN; >> +} >> + >> +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s) >> +{ >> + return s->i2cr & I2CR_IIEN; >> +} >> + >> +static inline bool imx_i2c_is_master(imx_i2c_state *s) >> +{ >> + return s->i2cr & I2CR_MSTA; >> +} >> + >> +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s) >> +{ >> + return s->i2cr & I2CR_MTX; >> +} >> + >> +static void imx_i2c_reset(DeviceState *dev) >> +{ >> + imx_i2c_state *s = IMX_I2C(dev); >> + >> + if (s->address != ADDR_RESET) { >> + i2c_end_transfer(s->bus); >> + } >> + >> + s->address = ADDR_RESET; >> + s->iadr = IADR_RESET; >> + s->ifdr = IFDR_RESET; >> + s->i2cr = I2CR_RESET; >> + s->i2sr = I2SR_RESET; >> + s->i2dr_read = I2DR_RESET; >> + s->i2dr_write = I2DR_RESET; >> +} >> + >> +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s) >> +{ >> + /* >> + * raise an interrupt if the device is enabled and it is configured >> + * to generate some interrupts. >> + */ >> + if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) { >> + s->i2sr |= I2SR_IIF; >> + qemu_irq_raise(s->irq); >> + } >> +} >> + >> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset, >> + unsigned size) >> +{ >> + uint16_t value; >> + imx_i2c_state *s = IMX_I2C(opaque); >> + >> + switch (offset) { >> + case IADR_ADDR: >> + value = s->iadr; >> + break; >> + case IFDR_ADDR: >> + value = s->ifdr; >> + break; >> + case I2CR_ADDR: >> + value = s->i2cr; >> + break; >> + case I2SR_ADDR: >> + value = s->i2sr; >> + break; >> + case I2DR_ADDR: >> + value = s->i2dr_read; >> + >> + if (imx_i2c_is_master(s)) { /* master mode */ >> + int ret = 0xff; >> + >> + if (s->address == ADDR_RESET) { >> + /* something is wrong as the address is not set */ >> + DPRINT("Trying to read without specifying the slave address\n"); >> + } else if (s->i2cr & I2CR_MTX) { >> + DPRINT("Trying to read but MTX is set\n"); >> + } else { >> + /* get the next byte */ >> + ret = i2c_recv(s->bus); >> + >> + if (ret >= 0) { >> + imx_i2c_raise_interrupt(s); >> + } else { >> + DPRINT("read failed for device 0x%02x\n" s->address); >> + ret = 0xff; >> + } >> + } >> + >> + s->i2dr_read = ret; >> + } >> + break; >> + default: >> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset); >> + break; >> + } >> + >> + DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset) > General question for the list, are we encouraging the use or PRIx in > new code to remove the needs for casts from uint_XXt to unsigned in > printfery? > , >> + (unsigned int)offset, value); >> + >> + return (uint64_t)value; >> +} >> + >> +static void imx_i2c_write(void *opaque, hwaddr offset, >> + uint64_t value, unsigned size) >> +{ >> + imx_i2c_state *s = IMX_I2C(opaque); >> + >> + DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset), >> + (unsigned int)offset, (int)value); >> + >> + value &= 0xff; >> + >> + switch (offset) { >> + case IADR_ADDR: >> + s->iadr = value & IADR_MASK; >> + /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */ >> + break; >> + case IFDR_ADDR: >> + s->ifdr = value & IFDR_MASK; >> + break; >> + case I2CR_ADDR: >> + if (imx_i2c_is_enabled(s) && ((value & I2CR_IEN) == 0)) { >> + /* This is a soft reset. IADR is preserved during soft resets */ >> + uint16_t iadr = s->iadr; >> + imx_i2c_reset(&s->parent_obj.qdev); >> + s->iadr = iadr; >> + } else { /* normal write */ >> + s->i2cr = value & I2CR_MASK; >> + >> + if (imx_i2c_is_master(s)) { /* master mode */ >> + /* set the bus to busy */ >> + s->i2sr |= I2SR_IBB; >> + } else { /* slave mode */ >> + /* bus is not busy anymore */ >> + s->i2sr &= ~I2SR_IBB; >> + >> + /* >> + *if we unset the master mode then it ends the ongoing >> + * transfer if any >> + */ >> + if (s->address != ADDR_RESET) { >> + i2c_end_transfer(s->bus); >> + s->address = ADDR_RESET; >> + } >> + } >> + >> + if (s->i2cr & I2CR_RSTA) { /* Restart */ >> + /* if this is a restart then it ends the ongoing transfer */ >> + if (s->address != ADDR_RESET) { >> + i2c_end_transfer(s->bus); >> + s->address = ADDR_RESET; >> + s->i2cr &= ~I2CR_RSTA; >> + } >> + } >> + } >> + break; >> + case I2SR_ADDR: >> + /* >> + * if the user writes 0 to IIF then lower the interrupt and >> + * reset the bit >> + */ >> + if ((s->i2sr & I2SR_IIF) && !(value & I2SR_IIF)) { >> + s->i2sr &= ~I2SR_IIF; >> + qemu_irq_lower(s->irq); >> + } >> + >> + /* >> + * if the user writes 0 to IAL, reset the bit >> + */ >> + if ((s->i2sr & I2SR_IAL) && !(value & I2SR_IAL)) { >> + s->i2sr &= ~I2SR_IAL; >> + } >> + >> + break; >> + case I2DR_ADDR: >> + /* if the device is not enabled, nothing to do */ >> + if (!imx_i2c_is_enabled(s)) { >> + break; >> + } >> + >> + s->i2dr_write = value & I2DR_MASK; >> + >> + if (imx_i2c_is_master(s)) { /* master mode */ >> + /* If this is the first write cycle then it is the slave addr */ >> + if (s->address == ADDR_RESET) { >> + if (i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1, > You could use an extract macro. extract32 is generally considered > better than shift and & logicvfor multi-bit fields. Ill have to look > up if extract16 exists (im away from tree for this review) no extract16() macro spotted. Should one be added? > >> + (int)s->i2dr_write & 0x01)) { >> + /* if non zero is returned, the adress is not valid */ >> + s->i2sr |= I2SR_RXAK; >> + } else { >> + s->address = s->i2dr_write; >> + s->i2sr &= ~I2SR_RXAK; >> + imx_i2c_raise_interrupt(s); >> + } >> + } else { /* This is a normal data write */ >> + if (i2c_send(s->bus, s->i2dr_write)) { >> + /* if the target return non zero then end the transfer */ >> + s->i2sr |= I2SR_RXAK; >> + s->address = ADDR_RESET; >> + i2c_end_transfer(s->bus); >> + } else { >> + s->i2sr &= ~I2SR_RXAK; >> + imx_i2c_raise_interrupt(s); >> + } >> + } >> + } >> + break; >> + default: >> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset); >> + break; >> + } >> +} >> + >> +static const MemoryRegionOps imx_i2c_ops = { >> + .read = imx_i2c_read, >> + .write = imx_i2c_write, >> + .valid.min_access_size = 1, >> + .valid.max_access_size = 2, >> + .endianness = DEVICE_NATIVE_ENDIAN, >> +}; >> + >> +static const VMStateDescription imx_i2c_vmstate = { >> + .name = TYPE_IMX_I2C, >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT16(address, imx_i2c_state), >> + VMSTATE_UINT16(iadr, imx_i2c_state), >> + VMSTATE_UINT16(ifdr, imx_i2c_state), >> + VMSTATE_UINT16(i2cr, imx_i2c_state), >> + VMSTATE_UINT16(i2sr, imx_i2c_state), >> + VMSTATE_UINT16(i2dr_read, imx_i2c_state), >> + VMSTATE_UINT16(i2dr_write, imx_i2c_state), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +static void imx_i2c_realize(DeviceState *dev, Error **errp) >> +{ >> + imx_i2c_state *s = IMX_I2C(dev); > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > to avoid the repeated casts below (and dereferencing of inline macros > although that has another solution i've mentioned below). This rule is > a little hot off the press and Andreas only just updated the > QOMConventions page this week to clarify this better. OK > >> + >> + memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C, >> + IMX_I2C_MEM_SIZE); >> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); >> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); >> + s->bus = i2c_init_bus(&SYS_BUS_DEVICE(dev)->qdev, "i2c"); > The &foo->qdev is the old style dereferencing downcast which we are > trying to eliminate. Instead this line should actually just be: > > s->bus = i2c_init_bus(DEVICE(dev), "i2c"); > > Consider the ->qdev (or parent_obj) field poisoned globally for all QOM objects. OK > > Regards, > Peter > >> +} >> + >> +static void imx_i2c_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->vmsd = &imx_i2c_vmstate; >> + dc->reset = imx_i2c_reset; >> + dc->realize = imx_i2c_realize; >> +} >> + >> +static const TypeInfo imx_i2c_type_info = { >> + .name = TYPE_IMX_I2C, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(imx_i2c_state), >> + .class_init = imx_i2c_class_init, >> +}; >> + >> +static void imx_i2c_register_types(void) >> +{ >> + type_register_static(&imx_i2c_type_info); >> +} >> + >> +type_init(imx_i2c_register_types) >> diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h >> index ea9e093..a875171 100644 >> --- a/include/hw/arm/imx.h >> +++ b/include/hw/arm/imx.h >> @@ -30,5 +30,6 @@ void imx_timerg_create(const hwaddr addr, >> qemu_irq irq, >> DeviceState *ccm); >> >> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq); >> >> #endif /* IMX_H */ >> -- >> 1.8.1.2 >> >>
On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: > On 05/05/2013 05:14 AM, Peter Crosthwaite wrote: >> You could use an extract macro. extract32 is generally considered >> better than shift and & logicvfor multi-bit fields. Ill have to look >> up if extract16 exists (im away from tree for this review) > > > no extract16() macro spotted. > Should one be added? There's no need for one -- just use extract32. The only reason for having a separate extract64 is to avoid doing 64 bit arithmetic when we don't have to, I think. -- PMM
Hi Peter, On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: >> On 05/05/2013 05:14 AM, Peter Crosthwaite wrote: >>> You could use an extract macro. extract32 is generally considered >>> better than shift and & logicvfor multi-bit fields. Ill have to look >>> up if extract16 exists (im away from tree for this review) >> >> >> no extract16() macro spotted. >> Should one be added? > > There's no need for one -- just use extract32. The only > reason for having a separate extract64 is to avoid doing > 64 bit arithmetic when we don't have to, I think. > perhaps a quick: #define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c)) would keep the 16bit device code explicit and clean? Save on dummy casts in certain situations as well (but not this one). Regards, Peter > -- PMM >
Am 05.05.2013 05:14, schrieb Peter Crosthwaite: > On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS > <jcd@tribudubois.net> wrote: >> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >> new file mode 100644 >> index 0000000..5b0d046 >> --- /dev/null >> +++ b/hw/i2c/imx_i2c.c [...] >> +typedef struct imx_i2c_state { > > types should be in CamelCase IMXI2CState > >> + SysBusDevice parent_obj; While at it, please add a white line here. Background is that this parent field will pretty likely go away once we switch to a better object-orientation framework - if it were in a header we would annotate it as private and thus hidden from documentation. >> + MemoryRegion iomem; >> + i2c_bus *bus; Please rather use i2c_bus bus; and qbus_create_inline() in instance_init. >> + qemu_irq irq; >> + >> + uint16_t address; >> + >> + uint16_t iadr; >> + uint16_t ifdr; >> + uint16_t i2cr; >> + uint16_t i2sr; >> + uint16_t i2dr_read; >> + uint16_t i2dr_write; >> +} imx_i2c_state; [...] >> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset, >> + unsigned size) >> +{ >> + uint16_t value; >> + imx_i2c_state *s = IMX_I2C(opaque); >> + >> + switch (offset) { >> + case IADR_ADDR: >> + value = s->iadr; >> + break; >> + case IFDR_ADDR: >> + value = s->ifdr; >> + break; >> + case I2CR_ADDR: >> + value = s->i2cr; >> + break; >> + case I2SR_ADDR: >> + value = s->i2sr; >> + break; >> + case I2DR_ADDR: >> + value = s->i2dr_read; >> + >> + if (imx_i2c_is_master(s)) { /* master mode */ >> + int ret = 0xff; >> + >> + if (s->address == ADDR_RESET) { >> + /* something is wrong as the address is not set */ >> + DPRINT("Trying to read without specifying the slave address\n"); >> + } else if (s->i2cr & I2CR_MTX) { >> + DPRINT("Trying to read but MTX is set\n"); >> + } else { >> + /* get the next byte */ >> + ret = i2c_recv(s->bus); >> + >> + if (ret >= 0) { >> + imx_i2c_raise_interrupt(s); >> + } else { >> + DPRINT("read failed for device 0x%02x\n" s->address); >> + ret = 0xff; >> + } >> + } >> + >> + s->i2dr_read = ret; >> + } >> + break; >> + default: >> + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset); >> + break; >> + } >> + >> + DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset) > > General question for the list, are we encouraging the use or PRIx in > new code to remove the needs for casts from uint_XXt to unsigned in > printfery? > , >> + (unsigned int)offset, value); I believe so, I've been asked to use HWADDR_PRIx macro in ppc patches. >> + >> + return (uint64_t)value; >> +} [snip] Cheers, Andreas
On 5 May 2013 11:53, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: >>> no extract16() macro spotted. >>> Should one be added? >> >> There's no need for one -- just use extract32. The only >> reason for having a separate extract64 is to avoid doing >> 64 bit arithmetic when we don't have to, I think. >> > > perhaps a quick: > > #define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c)) > > would keep the 16bit device code explicit and clean? Save on > dummy casts in certain situations as well (but not this one). Hmm, what situations would ever require a cast of the result or the input of extract32? -- PMM
Hi Peter, On Sun, May 5, 2013 at 9:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 May 2013 11:53, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: >>>> no extract16() macro spotted. >>>> Should one be added? >>> >>> There's no need for one -- just use extract32. The only >>> reason for having a separate extract64 is to avoid doing >>> 64 bit arithmetic when we don't have to, I think. >>> >> >> perhaps a quick: >> >> #define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c)) >> >> would keep the 16bit device code explicit and clean? Save on >> dummy casts in certain situations as well (but not this one). > > Hmm, what situations would ever require a cast of the result > or the input of extract32? > The one off the top of my head: fprintf(stderr, "your 16 field is :%" PRIx16 "\n", extract16(foo, bar, baz)); Otherwise you would have to match PRIx32 to extract 16 which is clumsy. But aren't there there some other varargs situations that may require casts as well? As for the input cast, I got nothin, I just put it in there for completeness. Meet you half way and drop the input cast and keep the output? Regards, Peter > -- PMM >
On 05/05/2013 01:53 PM, Peter Crosthwaite wrote: > Hi Peter, > > On Sun, May 5, 2013 at 9:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 5 May 2013 11:53, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >>> On Sun, May 5, 2013 at 8:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> On 5 May 2013 04:58, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: >>>>> no extract16() macro spotted. >>>>> Should one be added? >>>> There's no need for one -- just use extract32. The only >>>> reason for having a separate extract64 is to avoid doing >>>> 64 bit arithmetic when we don't have to, I think. >>>> >>> perhaps a quick: >>> >>> #define extract16(a, b, c) (uint16_t)extract32((uint16_t)(a), (b), (c)) >>> >>> would keep the 16bit device code explicit and clean? Save on >>> dummy casts in certain situations as well (but not this one). >> Hmm, what situations would ever require a cast of the result >> or the input of extract32? >> > The one off the top of my head: > > fprintf(stderr, "your 16 field is :%" PRIx16 "\n", extract16(foo, bar, baz)); > > Otherwise you would have to match PRIx32 to extract 16 which is clumsy. > > But aren't there there some other varargs situations that may require > casts as well? > > As for the input cast, I got nothin, I just put it in there for > completeness. Meet you half way and drop the input cast and keep the > output? > > Regards, > Peter I will use extract32() for now. JC > >> -- PMM >>
On 05/05/2013 01:34 PM, Andreas Färber wrote: > Am 05.05.2013 05:14, schrieb Peter Crosthwaite: >> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS >> <jcd@tribudubois.net> wrote: >>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c >>> new file mode 100644 >>> index 0000000..5b0d046 >>> --- /dev/null >>> +++ b/hw/i2c/imx_i2c.c > [...] >>> +typedef struct imx_i2c_state { >> types should be in CamelCase IMXI2CState >> >>> + SysBusDevice parent_obj; > While at it, please add a white line here. Background is that this > parent field will pretty likely go away once we switch to a better > object-orientation framework - if it were in a header we would annotate > it as private and thus hidden from documentation. Will do. > >>> + MemoryRegion iomem; >>> + i2c_bus *bus; > Please rather use i2c_bus bus; and qbus_create_inline() in instance_init. I am using i2c_init_bus() which itself uses qbus_create(). Do you mean we should not use i2c_init_bus() but instead reimplement locally based on qbus_create_inline()? JC
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak index b3a0207..a20f112 100644 --- a/default-configs/arm-softmmu.mak +++ b/default-configs/arm-softmmu.mak @@ -81,3 +81,5 @@ CONFIG_VERSATILE_PCI=y CONFIG_VERSATILE_I2C=y CONFIG_SDHCI=y + +CONFIG_IMX_I2C=y diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs index 648278e..d27bbaa 100644 --- a/hw/i2c/Makefile.objs +++ b/hw/i2c/Makefile.objs @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o common-obj-$(CONFIG_APM) += pm_smbus.o common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o +common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o obj-$(CONFIG_OMAP) += omap_i2c.o diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c new file mode 100644 index 0000000..5b0d046 --- /dev/null +++ b/hw/i2c/imx_i2c.c @@ -0,0 +1,383 @@ +/* + * i.MX I2C Bus Serial Interface Emulation + * + * Copyright (C) 2013 Jean-Christophe Dubois. + * + * 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, see <http://www.gnu.org/licenses/>. + * + */ + +#include "hw/sysbus.h" +#include "hw/i2c/i2c.h" + +#ifndef IMX_I2C_DEBUG +#define IMX_I2C_DEBUG 0 +#endif + +#define TYPE_IMX_I2C "imx.i2c" +#define IMX_I2C(obj) \ + OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C) + +/* i.MX I2C memory map */ +#define IMX_I2C_MEM_SIZE 0x14 +#define IADR_ADDR 0x00 /* address register */ +#define IFDR_ADDR 0x04 /* frequency divider register */ +#define I2CR_ADDR 0x08 /* control register */ +#define I2SR_ADDR 0x0c /* status register */ +#define I2DR_ADDR 0x10 /* data register */ + +#define IADR_MASK 0xFE +#define IADR_RESET 0 + +#define IFDR_MASK 0x3F +#define IFDR_RESET 0 + +#define I2CR_IEN (1 << 7) +#define I2CR_IIEN (1 << 6) +#define I2CR_MSTA (1 << 5) +#define I2CR_MTX (1 << 4) +#define I2CR_TXAK (1 << 3) +#define I2CR_RSTA (1 << 2) +#define I2CR_MASK 0xFC +#define I2CR_RESET 0 + +#define I2SR_ICF (1 << 7) +#define I2SR_IAAF (1 << 6) +#define I2SR_IBB (1 << 5) +#define I2SR_IAL (1 << 4) +#define I2SR_SRW (1 << 2) +#define I2SR_IIF (1 << 1) +#define I2SR_RXAK (1 << 0) +#define I2SR_MASK 0xE9 +#define I2SR_RESET 0x81 + +#define I2DR_MASK 0xFF +#define I2DR_RESET 0 + +#define ADDR_RESET 0xFF00 + +#if IMX_I2C_DEBUG +#define DPRINT(fmt, args...) \ + do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0) + +static const char *imx_i2c_get_regname(unsigned offset) +{ + switch (offset) { + case IADR_ADDR: + return "IADR"; + case IFDR_ADDR: + return "IFDR"; + case I2CR_ADDR: + return "I2CR"; + case I2SR_ADDR: + return "I2SR"; + case I2DR_ADDR: + return "I2DR"; + default: + return "[?]"; + } +} +#else +#define DPRINT(fmt, args...) do { } while (0) +#endif + +typedef struct imx_i2c_state { + SysBusDevice parent_obj; + MemoryRegion iomem; + i2c_bus *bus; + qemu_irq irq; + + uint16_t address; + + uint16_t iadr; + uint16_t ifdr; + uint16_t i2cr; + uint16_t i2sr; + uint16_t i2dr_read; + uint16_t i2dr_write; +} imx_i2c_state; + +static inline bool imx_i2c_is_enabled(imx_i2c_state *s) +{ + return s->i2cr & I2CR_IEN; +} + +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s) +{ + return s->i2cr & I2CR_IIEN; +} + +static inline bool imx_i2c_is_master(imx_i2c_state *s) +{ + return s->i2cr & I2CR_MSTA; +} + +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s) +{ + return s->i2cr & I2CR_MTX; +} + +static void imx_i2c_reset(DeviceState *dev) +{ + imx_i2c_state *s = IMX_I2C(dev); + + if (s->address != ADDR_RESET) { + i2c_end_transfer(s->bus); + } + + s->address = ADDR_RESET; + s->iadr = IADR_RESET; + s->ifdr = IFDR_RESET; + s->i2cr = I2CR_RESET; + s->i2sr = I2SR_RESET; + s->i2dr_read = I2DR_RESET; + s->i2dr_write = I2DR_RESET; +} + +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s) +{ + /* + * raise an interrupt if the device is enabled and it is configured + * to generate some interrupts. + */ + if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) { + s->i2sr |= I2SR_IIF; + qemu_irq_raise(s->irq); + } +} + +static uint64_t imx_i2c_read(void *opaque, hwaddr offset, + unsigned size) +{ + uint16_t value; + imx_i2c_state *s = IMX_I2C(opaque); + + switch (offset) { + case IADR_ADDR: + value = s->iadr; + break; + case IFDR_ADDR: + value = s->ifdr; + break; + case I2CR_ADDR: + value = s->i2cr; + break; + case I2SR_ADDR: + value = s->i2sr; + break; + case I2DR_ADDR: + value = s->i2dr_read; + + if (imx_i2c_is_master(s)) { /* master mode */ + int ret = 0xff; + + if (s->address == ADDR_RESET) { + /* something is wrong as the address is not set */ + DPRINT("Trying to read without specifying the slave address\n"); + } else if (s->i2cr & I2CR_MTX) { + DPRINT("Trying to read but MTX is set\n"); + } else { + /* get the next byte */ + ret = i2c_recv(s->bus); + + if (ret >= 0) { + imx_i2c_raise_interrupt(s); + } else { + DPRINT("read failed for device 0x%02x\n" s->address); + ret = 0xff; + } + } + + s->i2dr_read = ret; + } + break; + default: + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset); + break; + } + + DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset), + (unsigned int)offset, value); + + return (uint64_t)value; +} + +static void imx_i2c_write(void *opaque, hwaddr offset, + uint64_t value, unsigned size) +{ + imx_i2c_state *s = IMX_I2C(opaque); + + DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset), + (unsigned int)offset, (int)value); + + value &= 0xff; + + switch (offset) { + case IADR_ADDR: + s->iadr = value & IADR_MASK; + /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */ + break; + case IFDR_ADDR: + s->ifdr = value & IFDR_MASK; + break; + case I2CR_ADDR: + if (imx_i2c_is_enabled(s) && ((value & I2CR_IEN) == 0)) { + /* This is a soft reset. IADR is preserved during soft resets */ + uint16_t iadr = s->iadr; + imx_i2c_reset(&s->parent_obj.qdev); + s->iadr = iadr; + } else { /* normal write */ + s->i2cr = value & I2CR_MASK; + + if (imx_i2c_is_master(s)) { /* master mode */ + /* set the bus to busy */ + s->i2sr |= I2SR_IBB; + } else { /* slave mode */ + /* bus is not busy anymore */ + s->i2sr &= ~I2SR_IBB; + + /* + *if we unset the master mode then it ends the ongoing + * transfer if any + */ + if (s->address != ADDR_RESET) { + i2c_end_transfer(s->bus); + s->address = ADDR_RESET; + } + } + + if (s->i2cr & I2CR_RSTA) { /* Restart */ + /* if this is a restart then it ends the ongoing transfer */ + if (s->address != ADDR_RESET) { + i2c_end_transfer(s->bus); + s->address = ADDR_RESET; + s->i2cr &= ~I2CR_RSTA; + } + } + } + break; + case I2SR_ADDR: + /* + * if the user writes 0 to IIF then lower the interrupt and + * reset the bit + */ + if ((s->i2sr & I2SR_IIF) && !(value & I2SR_IIF)) { + s->i2sr &= ~I2SR_IIF; + qemu_irq_lower(s->irq); + } + + /* + * if the user writes 0 to IAL, reset the bit + */ + if ((s->i2sr & I2SR_IAL) && !(value & I2SR_IAL)) { + s->i2sr &= ~I2SR_IAL; + } + + break; + case I2DR_ADDR: + /* if the device is not enabled, nothing to do */ + if (!imx_i2c_is_enabled(s)) { + break; + } + + s->i2dr_write = value & I2DR_MASK; + + if (imx_i2c_is_master(s)) { /* master mode */ + /* If this is the first write cycle then it is the slave addr */ + if (s->address == ADDR_RESET) { + if (i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1, + (int)s->i2dr_write & 0x01)) { + /* if non zero is returned, the adress is not valid */ + s->i2sr |= I2SR_RXAK; + } else { + s->address = s->i2dr_write; + s->i2sr &= ~I2SR_RXAK; + imx_i2c_raise_interrupt(s); + } + } else { /* This is a normal data write */ + if (i2c_send(s->bus, s->i2dr_write)) { + /* if the target return non zero then end the transfer */ + s->i2sr |= I2SR_RXAK; + s->address = ADDR_RESET; + i2c_end_transfer(s->bus); + } else { + s->i2sr &= ~I2SR_RXAK; + imx_i2c_raise_interrupt(s); + } + } + } + break; + default: + hw_error("%s: Bad address 0x%x\n", __func__, (int)offset); + break; + } +} + +static const MemoryRegionOps imx_i2c_ops = { + .read = imx_i2c_read, + .write = imx_i2c_write, + .valid.min_access_size = 1, + .valid.max_access_size = 2, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static const VMStateDescription imx_i2c_vmstate = { + .name = TYPE_IMX_I2C, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT16(address, imx_i2c_state), + VMSTATE_UINT16(iadr, imx_i2c_state), + VMSTATE_UINT16(ifdr, imx_i2c_state), + VMSTATE_UINT16(i2cr, imx_i2c_state), + VMSTATE_UINT16(i2sr, imx_i2c_state), + VMSTATE_UINT16(i2dr_read, imx_i2c_state), + VMSTATE_UINT16(i2dr_write, imx_i2c_state), + VMSTATE_END_OF_LIST() + } +}; + +static void imx_i2c_realize(DeviceState *dev, Error **errp) +{ + imx_i2c_state *s = IMX_I2C(dev); + + memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C, + IMX_I2C_MEM_SIZE); + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); + s->bus = i2c_init_bus(&SYS_BUS_DEVICE(dev)->qdev, "i2c"); +} + +static void imx_i2c_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->vmsd = &imx_i2c_vmstate; + dc->reset = imx_i2c_reset; + dc->realize = imx_i2c_realize; +} + +static const TypeInfo imx_i2c_type_info = { + .name = TYPE_IMX_I2C, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(imx_i2c_state), + .class_init = imx_i2c_class_init, +}; + +static void imx_i2c_register_types(void) +{ + type_register_static(&imx_i2c_type_info); +} + +type_init(imx_i2c_register_types) diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h index ea9e093..a875171 100644 --- a/include/hw/arm/imx.h +++ b/include/hw/arm/imx.h @@ -30,5 +30,6 @@ void imx_timerg_create(const hwaddr addr, qemu_irq irq, DeviceState *ccm); +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq); #endif /* IMX_H */
The slave mode is not implemented. Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net> --- default-configs/arm-softmmu.mak | 2 + hw/i2c/Makefile.objs | 1 + hw/i2c/imx_i2c.c | 383 ++++++++++++++++++++++++++++++++++++++++ include/hw/arm/imx.h | 1 + 4 files changed, 387 insertions(+) create mode 100644 hw/i2c/imx_i2c.c