| Message ID | 1367438026-27573-1-git-send-email-jcd@tribudubois.net |
|---|---|
| State | New |
| Headers | show |
Hi Jean-Christophe, Thanks for your contribution. Please run the patch through scripts/checkpatch.pl to check for formatting errors. On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: > 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 | 374 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 377 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..30d3f5c > --- /dev/null > +++ b/hw/i2c/imx_i2c.c > @@ -0,0 +1,374 @@ > +/* > + * 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 busdev; > + 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); ERROR: return is not a function, parentheses are not required #148: FILE: hw/i2c/imx_i2c.c:114: + return (s->i2cr & I2CR_IEN); From checkpatch, here and below. > +} > + > +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 *d) > +{ > + imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d)); > + Please don't use FROM_SYSBUS in new code. Use QOM cast macros. http://wiki.qemu.org/QOMConventions Has useful guidelines for the rules around this for new devices. > + 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) > +{ > + imx_i2c_state *s = (imx_i2c_state *)opaque; QOM cast macro. > + uint16_t value; > + > + 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 Do not use C style comments. (Checkpatch picks then up) > + int ret; > + > + if (s->address == ADDR_RESET) { > + // something is wrong as the address is not set > + DPRINT("Trying to read with specifying the slave address\n"); > + } > + > + if ((ret = i2c_recv(s->bus)) >= 0) { > + s->i2dr_read = ret; > + imx_i2c_raise_interrupt(s); > + } else { > + DPRINT("read failed for device 0x%02x\n" s->address); > + s->i2dr_read = 0xff; > + } > + } else { // slave mode > + } Empty else not required. > + 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_state *)opaque; > + uint16_t v = value & 0xff; > + > + DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset), > + (unsigned int)offset, v); > + > + switch (offset) { > + case IADR_ADDR: > + s->iadr = v & IADR_MASK; > + //i2c_set_slave_address(s->bus, (uint8_t)s->iadr); > + break; > + case IFDR_ADDR: > + s->ifdr = v & IFDR_MASK; > + break; > + case I2CR_ADDR: > + if (imx_i2c_is_enabled(s) && ((v & I2CR_IEN) == 0)) { > + /* This is a soft reset. IADR is preserved during soft resets */ > + uint16_t iadr = s->iadr; > + imx_i2c_reset(&s->busdev.qdev); > + s->iadr = iadr; > + } else { // normal write > + s->i2cr = v & 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) && !(v & 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) && !(v & I2SR_IAL)) { > + s->i2sr &= ~I2SR_IAL; > + /* Nothing more to do ? */ > + } > + > + break; > + case I2DR_ADDR: > + /* if the device is not enabled, nothing to do */ > + if (!imx_i2c_is_enabled(s)) { > + break; > + } > + > + s->i2dr_write = v & 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 0 is returned, the adress is valid > + s->address = s->i2dr_write; > + s->i2sr &= ~I2SR_RXAK; > + imx_i2c_raise_interrupt(s); > + } else { // can't start i2c transaction > + } > + } 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 > + i2c_end_transfer(s->bus); > + } > + s->i2sr &= ~I2SR_RXAK; > + imx_i2c_raise_interrupt(s); > + } > + } else { // slave mode > + } > + 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, > + .endianness = DEVICE_NATIVE_ENDIAN, I think you may need a .valid definition here as it looks like you have restrictions on access size and alignment? (looks like 4bytes accesses only). > +}; > + > +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 int imx_i2c_init(SysBusDevice *dev) > +{ Use of the SysBusDeviceClass::init function is deprecated. Please use DeviceClass::realise or Object::init. With no reliance on properties I would suggest this one can be done as just an Object::init fn. Regards, Peter > + 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(dev, &s->iomem); > + sysbus_init_irq(dev, &s->irq); > + s->bus = i2c_init_bus(&dev->qdev, "i2c"); > + > + return 0; > +} > + > +static void imx_i2c_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + SysBusDeviceClass *sbdc = SYS_BUS_DEVICE_CLASS(klass); > + > + dc->vmsd = &imx_i2c_vmstate; > + dc->reset = imx_i2c_reset; > + sbdc->init = imx_i2c_init; > +} > + > +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); > -- > 1.8.1.2 > >
Hi, Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS: > 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 | 374 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 377 insertions(+) > create mode 100644 hw/i2c/imx_i2c.c Please thread your messages together so they can be reviewed in context. Since you're adding a new I2C device and we have a qtest framework for I2C, please supply an implementation for this device (which will require some constant sharing via header file) and some simple test case for the board you're using it on, to assure it keeps working. In addition to Peter C.'s comments: [...] > diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c > new file mode 100644 > index 0000000..30d3f5c > --- /dev/null > +++ b/hw/i2c/imx_i2c.c [...] > +type_init(imx_i2c_register_types); This declares a function and is not a statement, so no semicolon please. Regards, Andreas
Peter, Thanks for you review. I have a few questions below. JC On 05/02/2013 02:16 PM, Peter Crosthwaite wrote: > Hi Jean-Christophe, > > Thanks for your contribution. Please run the patch through > scripts/checkpatch.pl to check for formatting errors. > > On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS > <jcd@tribudubois.net> wrote: > ERROR: return is not a function, parentheses are not required > #148: FILE: hw/i2c/imx_i2c.c:114: > + return (s->i2cr & I2CR_IEN); > > >From checkpatch, here and below. Will do. >> +} >> + >> +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 *d) >> +{ >> + imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d)); >> + > Please don't use FROM_SYSBUS in new code. Use QOM cast macros. > > http://wiki.qemu.org/QOMConventions > > Has useful guidelines for the rules around this for new devices. It is not very clear what you do expect. Could you point me to a driver that is up to date. >> + 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) >> +{ >> + imx_i2c_state *s = (imx_i2c_state *)opaque; > QOM cast macro. Could you point me to an up to date driver using QOM cast macro? > > + > +static const MemoryRegionOps imx_i2c_ops = { > + .read = imx_i2c_read, > + .write = imx_i2c_write, > + .endianness = DEVICE_NATIVE_ENDIAN, > I think you may need a .valid definition here as it looks like you > have restrictions on access size and alignment? (looks like 4bytes > accesses only). I'll look into this. >> +}; >> + >> +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 int imx_i2c_init(SysBusDevice *dev) >> +{ > Use of the SysBusDeviceClass::init function is deprecated. Please use > DeviceClass::realise or Object::init. With no reliance on properties I > would suggest this one can be done as just an Object::init fn. Could you point me to the documentation or an up to date example?
Am 02.05.2013 20:33, schrieb Jean-Christophe DUBOIS: > On 05/02/2013 02:16 PM, Peter Crosthwaite wrote: >> On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS >> <jcd@tribudubois.net> wrote: >>> +static int imx_i2c_init(SysBusDevice *dev) >>> +{ >> Use of the SysBusDeviceClass::init function is deprecated. Please use >> DeviceClass::realise or Object::init. With no reliance on properties I >> would suggest this one can be done as just an Object::init fn. > Could you point me to the documentation or an up to date example? Documentation is in include/hw/qdev-core.h and include/qom/object.h. Andreas
Andreas, On 05/02/2013 02:38 PM, Andreas Färber wrote: > Hi, > > Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS: >> 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 | 374 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 377 insertions(+) >> create mode 100644 hw/i2c/imx_i2c.c > Please thread your messages together so they can be reviewed in context. > > Since you're adding a new I2C device and we have a qtest framework for > I2C, please supply an implementation for this device (which will require > some constant sharing via header file) and some simple test case for the > board you're using it on, to assure it keeps working. What is the "target" for building qtest? And then how to run it? Could you point me to some documentation please? It doesn't build out of the box for me. JC
Am 03.05.2013 17:16, schrieb Jean-Christophe DUBOIS: > Andreas, > > On 05/02/2013 02:38 PM, Andreas Färber wrote: >> Hi, >> >> Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS: >>> 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 | 374 >>> ++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 377 insertions(+) >>> create mode 100644 hw/i2c/imx_i2c.c >> Please thread your messages together so they can be reviewed in context. >> >> Since you're adding a new I2C device and we have a qtest framework for >> I2C, please supply an implementation for this device (which will require >> some constant sharing via header file) and some simple test case for the >> board you're using it on, to assure it keeps working. > What is the "target" for building qtest? > > And then how to run it? make check Andreas > Could you point me to some documentation please? It doesn't build out of > the box for me. > > JC >
On 05/03/2013 05:20 PM, Andreas Färber wrote: > Am 03.05.2013 17:16, schrieb Jean-Christophe DUBOIS: >> Andreas, >> >> On 05/02/2013 02:38 PM, Andreas Färber wrote: >>> Hi, >>> >>> Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS: >>>> 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 | 374 >>>> ++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 377 insertions(+) >>>> create mode 100644 hw/i2c/imx_i2c.c >>> Please thread your messages together so they can be reviewed in context. >>> >>> Since you're adding a new I2C device and we have a qtest framework for >>> I2C, please supply an implementation for this device (which will require >>> some constant sharing via header file) and some simple test case for the >>> board you're using it on, to assure it keeps working. >> What is the "target" for building qtest? >> >> And then how to run it? > make check Thanks Andreas, So I added a libi2c-imx.c file, a ds1338-test.c file ... but when I run "make check" I get the following. What am I missing? JC $ make check GTESTER tests/check-qdict GTESTER tests/check-qfloat GTESTER tests/check-qint GTESTER tests/check-qstring GTESTER tests/check-qlist GTESTER tests/check-qjson GTESTER tests/test-qmp-output-visitor GTESTER tests/test-qmp-input-visitor GTESTER tests/test-qmp-input-strict GTESTER tests/test-qmp-commands GTESTER tests/test-string-input-visitor GTESTER tests/test-string-output-visitor GTESTER tests/test-coroutine GTESTER tests/test-visitor-serialization GTESTER tests/test-iov GTESTER tests/test-aio GTESTER tests/test-thread-pool GTESTER tests/test-hbitmap GTESTER tests/test-x86-cpuid GTESTER tests/test-xbzrle GTESTER tests/test-cutils GTESTER tests/test-mul64 CC tests/libi2c-imx.o LINK tests/tmp105-test CC tests/ds1338-test.o LINK tests/ds1338-test GTESTER check-qtest-arm Kernel image must be specified Broken pipe make: *** [check-qtest-arm] Error 1 $ > > Andreas > >> Could you point me to some documentation please? It doesn't build out of >> the box for me. >> >> JC >> >
Am 03.05.2013 18:04, schrieb Jean-Christophe DUBOIS: > So I added a libi2c-imx.c file, a ds1338-test.c file ... (Note that in light of http://patchwork.ozlabs.org/patch/241004/ this will likely become libqos/i2c-imx.c.) > but when I run "make check" I get the following. [...] > $ make check [...] > CC tests/libi2c-imx.o > LINK tests/tmp105-test > CC tests/ds1338-test.o > LINK tests/ds1338-test > GTESTER check-qtest-arm > Kernel image must be specified Without seeing your code and since tmp105-test is working on my side, I can only guess that your ds1338-test is starting either your own machine or some machine that terminates with above error. The solution is to only mandate -kernel, -initrd, -dtb, etc. in the machine init function if !qtest_enabled() or so. > Broken pipe That's just qtest complaining that the process unexpectedly is gone due to the above. > make: *** [check-qtest-arm] Error 1 Note: For getting your test working you can skip the other tests above using either the specific name from the error or check-qtest: $ make check-help Andreas
On 05/03/2013 06:41 PM, Andreas Färber wrote: > CC tests/libi2c-imx.o > LINK tests/tmp105-test > CC tests/ds1338-test.o > LINK tests/ds1338-test > GTESTER check-qtest-arm > Kernel image must be specified > Without seeing your code and since tmp105-test is working on my side, I > can only guess that your ds1338-test is starting either your own machine > or some machine that terminates with above error. the ds1338-test.c code is very similar to the tmp105-test.c code. The tmp105-test.c code starts a n800 machine while the ds1338-test.c starts a imx25_3ds machine (to get the i.MX I2C device, it seems natural). Nowhere in the tmp105-test.c code is any kernel or initrd file specified. So I did the same with ds1338-test.c. But then "make check" is complaining the kernel is missing ... So it seems the n800 allows qemu to start even if no parameters (ram size, kernel file, ...) are provided (you can actually confirm this by running "qemu-system-arm -machine n800"). This is not the default behavior for most implemented platform (including imx25_3ds) that will refuse to start if no parameter is given. Is there a new requirement mandating platforms to start with guessed parameters if not provided on the command line in order to be compatible with qtest? > > The solution is to only mandate -kernel, -initrd, -dtb, etc. in the > machine init function if !qtest_enabled() or so. I am calling arm_load_kernel() with a pointer to a "struct arm_boot_info". The first thing done by arm_load_kernel() is to check for the kernel parameter without checking for qtest_enable() and all. /* Load the kernel. */ if (!info->kernel_filename) { fprintf(stderr, "Kernel image must be specified\n"); exit(1); } Do you mean arm_load_kernel() should test for qtest_enable() or should I call arm_load_kernel() only if !qtest_enable() (there is not much platform checking for qtest_enable() at this time and it seems somebody need to set the "entry" field in the info struct).
Am 04.05.2013 10:22, schrieb Jean-Christophe DUBOIS: > On 05/03/2013 06:41 PM, Andreas Färber wrote: >> CC tests/libi2c-imx.o >> LINK tests/tmp105-test >> CC tests/ds1338-test.o >> LINK tests/ds1338-test >> GTESTER check-qtest-arm >> Kernel image must be specified >> Without seeing your code and since tmp105-test is working on my side, I >> can only guess that your ds1338-test is starting either your own machine >> or some machine that terminates with above error. > the ds1338-test.c code is very similar to the tmp105-test.c code. > > The tmp105-test.c code starts a n800 machine while the ds1338-test.c > starts a imx25_3ds machine (to get the i.MX I2C device, it seems natural). > > Nowhere in the tmp105-test.c code is any kernel or initrd file > specified. So I did the same with ds1338-test.c. The mentioned machine init is part of the emulation, not of qtest. > But then "make check" is complaining the kernel is missing ... > > So it seems the n800 allows qemu to start even if no parameters (ram > size, kernel file, ...) are provided (you can actually confirm this by > running "qemu-system-arm -machine n800"). This is not the default > behavior for most implemented platform (including imx25_3ds) that will > refuse to start if no parameter is given. > > Is there a new requirement mandating platforms to start with guessed > parameters if not provided on the command line in order to be compatible > with qtest? >> >> The solution is to only mandate -kernel, -initrd, -dtb, etc. in the >> machine init function if !qtest_enabled() or so. > > I am calling arm_load_kernel() with a pointer to a "struct arm_boot_info". > > The first thing done by arm_load_kernel() is to check for the kernel > parameter without checking for qtest_enable() and all. > > /* Load the kernel. */ > if (!info->kernel_filename) { > fprintf(stderr, "Kernel image must be specified\n"); > exit(1); > } > > Do you mean arm_load_kernel() should test for qtest_enable() or should I > call arm_load_kernel() only if !qtest_enable() (there is not much > platform checking for qtest_enable() at this time and it seems somebody > need to set the "entry" field in the info struct). In short, qtest does not execute any code, so it does not need to load binaries or set CPU entry addresses. Yes, I would say there is no point calling arm_load_kernel() if !qtest_enabled(). Andreas
On 4 May 2013 09:27, Andreas Färber <afaerber@suse.de> wrote: > Am 04.05.2013 10:22, schrieb Jean-Christophe DUBOIS: >> Do you mean arm_load_kernel() should test for qtest_enable() or should I >> call arm_load_kernel() only if !qtest_enable() (there is not much >> platform checking for qtest_enable() at this time and it seems somebody >> need to set the "entry" field in the info struct). > > In short, qtest does not execute any code, so it does not need to load > binaries or set CPU entry addresses. > > Yes, I would say there is no point calling arm_load_kernel() if > !qtest_enabled(). I'd rather we didn't end up with qtest_enabled() checks infecting every board model, please... thanks -- PMM
On 05/04/2013 10:29 AM, Peter Maydell wrote: > On 4 May 2013 09:27, Andreas Färber <afaerber@suse.de> wrote: >> Am 04.05.2013 10:22, schrieb Jean-Christophe DUBOIS: >>> Do you mean arm_load_kernel() should test for qtest_enable() or should I >>> call arm_load_kernel() only if !qtest_enable() (there is not much >>> platform checking for qtest_enable() at this time and it seems somebody >>> need to set the "entry" field in the info struct). >> In short, qtest does not execute any code, so it does not need to load >> binaries or set CPU entry addresses. >> >> Yes, I would say there is no point calling arm_load_kernel() if >> !qtest_enabled(). > I'd rather we didn't end up with qtest_enabled() checks infecting > every board model, please... Then should we put it in arm_load_kernel() so that all board model get it ? JC > > thanks > -- PMM >
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..30d3f5c --- /dev/null +++ b/hw/i2c/imx_i2c.c @@ -0,0 +1,374 @@ +/* + * 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 busdev; + 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 *d) +{ + imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d)); + + 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) +{ + imx_i2c_state *s = (imx_i2c_state *)opaque; + uint16_t value; + + 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; + + if (s->address == ADDR_RESET) { + // something is wrong as the address is not set + DPRINT("Trying to read with specifying the slave address\n"); + } + + if ((ret = i2c_recv(s->bus)) >= 0) { + s->i2dr_read = ret; + imx_i2c_raise_interrupt(s); + } else { + DPRINT("read failed for device 0x%02x\n" s->address); + s->i2dr_read = 0xff; + } + } else { // slave mode + } + 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_state *)opaque; + uint16_t v = value & 0xff; + + DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset), + (unsigned int)offset, v); + + switch (offset) { + case IADR_ADDR: + s->iadr = v & IADR_MASK; + //i2c_set_slave_address(s->bus, (uint8_t)s->iadr); + break; + case IFDR_ADDR: + s->ifdr = v & IFDR_MASK; + break; + case I2CR_ADDR: + if (imx_i2c_is_enabled(s) && ((v & I2CR_IEN) == 0)) { + /* This is a soft reset. IADR is preserved during soft resets */ + uint16_t iadr = s->iadr; + imx_i2c_reset(&s->busdev.qdev); + s->iadr = iadr; + } else { // normal write + s->i2cr = v & 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) && !(v & 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) && !(v & I2SR_IAL)) { + s->i2sr &= ~I2SR_IAL; + /* Nothing more to do ? */ + } + + break; + case I2DR_ADDR: + /* if the device is not enabled, nothing to do */ + if (!imx_i2c_is_enabled(s)) { + break; + } + + s->i2dr_write = v & 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 0 is returned, the adress is valid + s->address = s->i2dr_write; + s->i2sr &= ~I2SR_RXAK; + imx_i2c_raise_interrupt(s); + } else { // can't start i2c transaction + } + } 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 + i2c_end_transfer(s->bus); + } + s->i2sr &= ~I2SR_RXAK; + imx_i2c_raise_interrupt(s); + } + } else { // slave mode + } + 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, + .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 int imx_i2c_init(SysBusDevice *dev) +{ + 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(dev, &s->iomem); + sysbus_init_irq(dev, &s->irq); + s->bus = i2c_init_bus(&dev->qdev, "i2c"); + + return 0; +} + +static void imx_i2c_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + SysBusDeviceClass *sbdc = SYS_BUS_DEVICE_CLASS(klass); + + dc->vmsd = &imx_i2c_vmstate; + dc->reset = imx_i2c_reset; + sbdc->init = imx_i2c_init; +} + +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);
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 | 374 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 377 insertions(+) create mode 100644 hw/i2c/imx_i2c.c