| Message ID | 1360143925-10800-21-git-send-email-dantesu@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Kuo-Jung, On Wed, Feb 6, 2013 at 7:45 PM, Kuo-Jung Su <dantesu@gmail.com> wrote: > From: Kuo-Jung Su <dantesu@faraday-tech.com> > > The FTSPI020 is an integrated SPI Flash controller > which supports upto 4 flash chips. > > Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com> > --- > hw/arm/Makefile.objs | 1 + > hw/arm/faraday_a369.c | 12 ++ > hw/arm/ftspi020.c | 345 +++++++++++++++++++++++++++++++++++++++++++++++++ > hw/arm/ftspi020.h | 50 +++++++ > 4 files changed, 408 insertions(+) > create mode 100644 hw/arm/ftspi020.c > create mode 100644 hw/arm/ftspi020.h > > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs > index 508335a..7178b5d 100644 > --- a/hw/arm/Makefile.objs > +++ b/hw/arm/Makefile.objs > @@ -52,3 +52,4 @@ obj-y += ftgmac100.o > obj-y += ftlcdc200.o > obj-y += fttsc010.o > obj-y += ftsdc010.o > +obj-y += ftspi020.o > diff --git a/hw/arm/faraday_a369.c b/hw/arm/faraday_a369.c > index 4ad48f5..132ee2f 100644 > --- a/hw/arm/faraday_a369.c > +++ b/hw/arm/faraday_a369.c > @@ -203,6 +203,18 @@ a369_device_init(A369State *s) > req = qdev_get_gpio_in(s->hdma[0], 13); > qdev_connect_gpio_out(s->hdma[0], 13, ack); > qdev_connect_gpio_out(ds, 0, req); > + > + /* ftspi020: as an external AHB device */ > + ds = sysbus_create_simple("ftspi020", 0xC0000000, pic[13]); > + spi = (SSIBus *)qdev_get_child_bus(ds, "spi"); > + nr_flash = 1; > + for (i = 0; i < nr_flash; i++) { > + fl = ssi_create_slave_no_init(spi, "m25p80"); > + qdev_prop_set_string(fl, "partname", "w25q64"); > + qdev_init_nofail(fl); > + cs_line = qdev_get_gpio_in(fl, 0); > + sysbus_connect_irq(SYS_BUS_DEVICE(ds), i + 1, cs_line); > + } > } > > static void > diff --git a/hw/arm/ftspi020.c b/hw/arm/ftspi020.c > new file mode 100644 > index 0000000..dab2f6f > --- /dev/null > +++ b/hw/arm/ftspi020.c > @@ -0,0 +1,345 @@ > +/* > + * Faraday FTSPI020 Flash Controller > + * > + * Copyright (c) 2012 Faraday Technology > + * Written by Dante Su <dantesu@faraday-tech.com> > + * > + * This code is licensed under GNU GPL v2+. > + */ > + > +#include <hw/hw.h> > +#include <sysemu/sysemu.h> > +#include <hw/sysbus.h> > +#include <hw/ssi.h> > + > +#include "ftspi020.h" > + > +#define TYPE_FTSPI020 "ftspi020" > + > +typedef struct Ftspi020State { > + SysBusDevice busdev; > + MemoryRegion iomem; > + qemu_irq irq; > + > + /* DMA hardware handshake */ > + qemu_irq req; > + > + SSIBus *spi; > + uint8_t num_cs; Constant. No need for it to be part of the device state, unless you want to drive it from a property and let the machine model decide how my cs lines you have? > + qemu_irq *cs_lines; > + > + int wip; /* SPI Flash Status: Write In Progress BIT shift */ > + uint32_t datacnt; > + > + /* HW register caches */ > + uint32_t cmd[4]; > + uint32_t ctrl; > + uint32_t timing; > + uint32_t icr; > + uint32_t isr; > + uint32_t rdsr; > +} Ftspi020State; > + > +#define FTSPI020(obj) \ > + OBJECT_CHECK(Ftspi020State, obj, TYPE_FTSPI020) > + > +static void ftspi020_update_irq(Ftspi020State *s) > +{ > + if (s->isr) { > + qemu_set_irq(s->irq, 1); > + } else { > + qemu_set_irq(s->irq, 0); > + } > +} > + > +static void ftspi020_handle_ack(void *opaque, int line, int level) > +{ > + Ftspi020State *s = FTSPI020(opaque); > + > + if (!(s->icr & 0x01)) { > + return; > + } > + > + if (level) { > + qemu_set_irq(s->req, 0); > + } else if (s->datacnt > 0) { > + qemu_set_irq(s->req, 1); > + } > +} > + > +static int ftspi020_do_command(Ftspi020State *s) > +{ > + int cs = (s->cmd[3] >> 8) & 0x03; > + int cmd = (s->cmd[3] >> 24) & 0xff; > + int ilen = (s->cmd[1] >> 24) & 0x03; > + int alen = (s->cmd[1] >> 0) & 0x07; > + int dcyc = (s->cmd[1] >> 16) & 0xff; > + bitops.h has helpers that can extract fields for you without all the >> & stuff. Lots of magic numbers here as well, can you #define these offsets with meaningful names? > + /* make sure the spi flash is de-activated */ > + qemu_set_irq(s->cs_lines[cs], 1); > + > + /* activate the spi flash */ > + qemu_set_irq(s->cs_lines[cs], 0); > + > + /* if it's a SPI flash READ_STATUS command */ > + if ((s->cmd[3] & 0x06) == 0x04) { > + do { > + ssi_transfer(s->spi, cmd); > + s->rdsr = ssi_transfer(s->spi, 0x00); > + if (s->cmd[3] & 0x08) { > + break; > + } > + } while (s->rdsr & (1 << s->wip)); > + } else { > + /* otherwise */ > + int i; > + > + ilen = MIN(ilen, 2); > + alen = MIN(alen, 4); > + > + /* command cycles */ > + for (i = 0; i < ilen; ++i) { > + ssi_transfer(s->spi, cmd); > + } > + /* address cycles */ > + for (i = alen - 1; i >= 0; --i) { > + ssi_transfer(s->spi, (s->cmd[0] >> (8 * i)) & 0xff); > + } > + /* dummy cycles */ > + for (i = 0; i < (dcyc >> 3); ++i) { > + ssi_transfer(s->spi, 0x00); > + } > + } > + > + if (s->datacnt <= 0) { == as this is unsigned. > + qemu_set_irq(s->cs_lines[cs], 1); > + } else if (s->icr & 0x01) { > + qemu_set_irq(s->req, 1); > + } > + > + if (s->cmd[3] & 0x01) { > + s->isr |= 0x01; > + } > + ftspi020_update_irq(s); > + > + return 0; > +} > + > +static void ftspi020_chip_reset(Ftspi020State *s) > +{ > + int i; > + > + s->datacnt = 0; > + for (i = 0; i < 4; ++i) { > + s->cmd[i] = 0; > + } > + s->wip = 0; > + s->ctrl = 0; > + s->timing = 0; > + s->icr = 0; > + s->isr = 0; > + s->rdsr = 0; > + > + qemu_set_irq(s->irq, 0); Do the cs lines need resetting here? > +} > + > +static uint64_t ftspi020_mem_read(void *opaque, hwaddr addr, unsigned size) > +{ > + Ftspi020State *s = FTSPI020(opaque); > + uint64_t ret = 0; > + > + switch (addr) { > + case REG_DATA: > + if (!(s->cmd[3] & 0x02)) { > + if (s->datacnt > 0) { > + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 0; > + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 8; > + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 16; > + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 24; loopable: for (i = 0; i < 4; ++i) { ret = deposit32(ret, i * 8, 8, (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff)); } > + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); > + } > + if (s->datacnt == 0) { > + uint8_t cs = (s->cmd[3] >> 8) & 0x03; > + qemu_set_irq(s->cs_lines[cs], 1); > + if (s->cmd[3] & 0x01) { > + s->isr |= 0x01; > + } > + ftspi020_update_irq(s); > + } > + } > + break; > + case REG_RDST: > + return s->rdsr; > + case REG_CMD0: the case .. syntax would allow you to collapse these 4 into 1: case REG_CMD0 .. REG_CMD3: return s->cmd[(addr - REG_CMD0) / 4] > + return s->cmd[0]; > + case REG_CMD1: > + return s->cmd[1]; > + case REG_CMD2: > + return s->cmd[2]; > + case REG_CMD3: > + return s->cmd[3]; > + case REG_STR: > + /* In QEMU, the data fifo is always ready for read/write */ > + return 0x00000003; > + case REG_ISR: > + return s->isr; > + case REG_ICR: > + return s->icr; > + case REG_CTRL: > + return s->ctrl; > + case REG_ACT: > + return s->timing; > + case REG_REV: > + return 0x00010001; > + case REG_FEA: > + return 0x02022020; Few magic numbers here > + default: > + break; > + } > + > + return ret; > +} > + > +static void ftspi020_mem_write(void *opaque, > + hwaddr addr, > + uint64_t val, > + unsigned size) > +{ > + Ftspi020State *s = FTSPI020(opaque); > + > + switch (addr) { > + case REG_DATA: > + if (s->cmd[3] & 0x02) { > + if (s->datacnt > 0) { > + ssi_transfer(s->spi, (uint8_t)((val >> 0) & 0xff)); > + ssi_transfer(s->spi, (uint8_t)((val >> 8) & 0xff)); > + ssi_transfer(s->spi, (uint8_t)((val >> 16) & 0xff)); > + ssi_transfer(s->spi, (uint8_t)((val >> 24) & 0xff)); > + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); > + } > + if (s->datacnt == 0) { > + uint8_t cs = (s->cmd[3] >> 8) & 0x03; > + qemu_set_irq(s->cs_lines[cs], 1); > + if (s->cmd[3] & 0x01) { > + s->isr |= 0x01; > + } > + ftspi020_update_irq(s); > + } > + } > + break; > + case REG_CMD0: > + s->cmd[0] = (uint32_t)val; > + break; > + case REG_CMD1: > + s->cmd[1] = (uint32_t)val; > + break; > + case REG_CMD2: > + s->datacnt = s->cmd[2] = (uint32_t)val; > + break; > + case REG_CMD3: > + s->cmd[3] = (uint32_t)val; > + ftspi020_do_command(s); > + break; > + case REG_ISR: > + s->isr &= ~((uint32_t)val); > + ftspi020_update_irq(s); > + break; > + case REG_ICR: > + s->icr = (uint32_t)val; > + break; > + case REG_CTRL: > + if (val & 0x100) { > + ftspi020_chip_reset(s); > + } > + s->ctrl = (uint32_t)val & 0x70013; > + s->wip = ((uint32_t)val >> 16) & 0x07; > + break; > + case REG_ACT: > + s->timing = (uint32_t)val; > + break; > + default: > + break; > + } > +} > + > +static const MemoryRegionOps ftspi020_ops = { > + .read = ftspi020_mem_read, > + .write = ftspi020_mem_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > +}; > + > +static void ftspi020_reset(DeviceState *ds) > +{ > + SysBusDevice *busdev = SYS_BUS_DEVICE(ds); > + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, busdev)); > + > + ftspi020_chip_reset(s); > +} > + > +static int ftspi020_init(SysBusDevice *dev) > +{ > + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, dev)); > + int i; > + > + memory_region_init_io(&s->iomem, > + &ftspi020_ops, > + s, > + TYPE_FTSPI020, > + 0x1000); > + sysbus_init_mmio(dev, &s->iomem); > + sysbus_init_irq(dev, &s->irq); > + > + s->spi = ssi_create_bus(&dev->qdev, "spi"); > + s->num_cs = 4; > + s->cs_lines = g_new(qemu_irq, s->num_cs); > + ssi_auto_connect_slaves(DEVICE(s), s->cs_lines, s->spi); > + for (i = 0; i < s->num_cs; ++i) { > + sysbus_init_irq(dev, &s->cs_lines[i]); > + } > + > + qdev_init_gpio_in(&s->busdev.qdev, ftspi020_handle_ack, 1); > + qdev_init_gpio_out(&s->busdev.qdev, &s->req, 1); > + > + return 0; > +} > + > +static const VMStateDescription vmstate_ftspi020 = { > + .name = TYPE_FTSPI020, > + .version_id = 1, > + .minimum_version_id = 1, > + .minimum_version_id_old = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32_ARRAY(cmd, Ftspi020State, 4), > + VMSTATE_UINT32(ctrl, Ftspi020State), > + VMSTATE_UINT32(timing, Ftspi020State), > + VMSTATE_UINT32(icr, Ftspi020State), > + VMSTATE_UINT32(isr, Ftspi020State), > + VMSTATE_UINT32(rdsr, Ftspi020State), datacnt is missing. I think you need to save it as it is device state that would need to be preserved if you machine got migrated in the middle of a transaction. > + VMSTATE_END_OF_LIST(), > + } > +}; > + > +static void ftspi020_class_init(ObjectClass *klass, void *data) > +{ > + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + k->init = ftspi020_init; > + dc->vmsd = &vmstate_ftspi020; > + dc->reset = ftspi020_reset; > + dc->no_user = 1; > +} > + > +static const TypeInfo ftspi020_info = { > + .name = TYPE_FTSPI020, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(Ftspi020State), > + .class_init = ftspi020_class_init, > +}; > + > +static void ftspi020_register_types(void) > +{ > + type_register_static(&ftspi020_info); > +} > + > +type_init(ftspi020_register_types) > diff --git a/hw/arm/ftspi020.h b/hw/arm/ftspi020.h > new file mode 100644 > index 0000000..a8a0930 > --- /dev/null > +++ b/hw/arm/ftspi020.h > @@ -0,0 +1,50 @@ > +/* > + * Faraday FTSPI020 Flash Controller > + * > + * Copyright (c) 2012 Faraday Technology > + * Written by Dante Su <dantesu@faraday-tech.com> > + * > + * This code is licensed under GNU GPL v2+. > + */ > + > +#ifndef HW_ARM_FTSPI020_H > +#define HW_ARM_FTSPI020_H > + This device is not exporting any extensible APIs there is no need for a header specific to this device. > +/****************************************************************************** > + * FTSPI020 registers > + *****************************************************************************/ > +#define REG_CMD0 0x00 /* Flash address */ > +#define REG_CMD1 0x04 > +#define REG_CMD2 0x08 > +#define REG_CMD3 0x0c > +#define REG_CTRL 0x10 /* Control Register */ > +#define REG_ACT 0x14 /* AC Timing Register */ > +#define REG_STR 0x18 /* Status Register */ > +#define REG_ICR 0x20 /* Interrupt Control Register */ > +#define REG_ISR 0x24 /* Interrupt Status Register */ > +#define REG_RDST 0x28 /* Read Status Register */ > +#define REG_REV 0x50 /* Revision Register */ > +#define REG_FEA 0x54 /* Feature Register */ > +#define REG_DATA 0x100 /* Data Register */ > + These can just live in the C file - they are private to the implementation. > +/****************************************************************************** > + * Common SPI flash opcodes (Not FTSPI020 specific) > + *****************************************************************************/ > +#define OPCODE_WREN 0x06 /* Write enable */ > +#define OPCODE_WRSR 0x01 /* Write status register 1 byte */ > +#define OPCODE_RDSR 0x05 /* Read status register */ > +#define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ > +#define OPCODE_NORM_READ4 0x13 /* Read data bytes (4 bytes address) */ > +#define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ > +#define OPCODE_FAST_READ4 0x0c /* Read data bytes (4 bytes address) */ > +#define OPCODE_PP 0x02 /* Page program (up to 256 bytes) */ > +#define OPCODE_PP4 0x12 /* Page program (4 bytes address) */ > +#define OPCODE_SE 0xd8 /* Sector erase (usually 64KiB) */ > +#define OPCODE_SE4 0xdc /* Sector erase (4 bytes address) */ > +#define OPCODE_RDID 0x9f /* Read JEDEC ID */ > + This is repetition of M25P80s command opcodes. If anything, we should factor these out into m25p80.h (doesnt exist yet) as this is the second device model (apart from m25p80 itself) to need these. The other one is xilinx_spips.c. However i notice almost all of these are unused by your device model. Apart from the little bit of logic around RDSR, is there any flash command-specific functionality in this device? AFAICT it just accepts command code, address, num dummies and payload size and the details of what those commands mean is abstracted away from this hardware, making this table mostly redundant. If you have future work that will need this that's another matter however. Regards, Peter > +/* Status Register bits. */ > +#define SR_WIP 1 /* Write in progress */ > +#define SR_WEL 2 /* Write enable latch */ > + > +#endif > -- > 1.7.9.5 > >
\ 2013/2/7 Peter Crosthwaite <peter.crosthwaite@xilinx.com>: > Hi Kuo-Jung, > > On Wed, Feb 6, 2013 at 7:45 PM, Kuo-Jung Su <dantesu@gmail.com> wrote: >> From: Kuo-Jung Su <dantesu@faraday-tech.com> >> >> The FTSPI020 is an integrated SPI Flash controller >> which supports upto 4 flash chips. >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com> >> --- >> hw/arm/Makefile.objs | 1 + >> hw/arm/faraday_a369.c | 12 ++ >> hw/arm/ftspi020.c | 345 +++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/arm/ftspi020.h | 50 +++++++ >> 4 files changed, 408 insertions(+) >> create mode 100644 hw/arm/ftspi020.c >> create mode 100644 hw/arm/ftspi020.h >> >> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs >> index 508335a..7178b5d 100644 >> --- a/hw/arm/Makefile.objs >> +++ b/hw/arm/Makefile.objs >> @@ -52,3 +52,4 @@ obj-y += ftgmac100.o >> obj-y += ftlcdc200.o >> obj-y += fttsc010.o >> obj-y += ftsdc010.o >> +obj-y += ftspi020.o >> diff --git a/hw/arm/faraday_a369.c b/hw/arm/faraday_a369.c >> index 4ad48f5..132ee2f 100644 >> --- a/hw/arm/faraday_a369.c >> +++ b/hw/arm/faraday_a369.c >> @@ -203,6 +203,18 @@ a369_device_init(A369State *s) >> req = qdev_get_gpio_in(s->hdma[0], 13); >> qdev_connect_gpio_out(s->hdma[0], 13, ack); >> qdev_connect_gpio_out(ds, 0, req); >> + >> + /* ftspi020: as an external AHB device */ >> + ds = sysbus_create_simple("ftspi020", 0xC0000000, pic[13]); >> + spi = (SSIBus *)qdev_get_child_bus(ds, "spi"); >> + nr_flash = 1; >> + for (i = 0; i < nr_flash; i++) { >> + fl = ssi_create_slave_no_init(spi, "m25p80"); >> + qdev_prop_set_string(fl, "partname", "w25q64"); >> + qdev_init_nofail(fl); >> + cs_line = qdev_get_gpio_in(fl, 0); >> + sysbus_connect_irq(SYS_BUS_DEVICE(ds), i + 1, cs_line); >> + } >> } >> >> static void >> diff --git a/hw/arm/ftspi020.c b/hw/arm/ftspi020.c >> new file mode 100644 >> index 0000000..dab2f6f >> --- /dev/null >> +++ b/hw/arm/ftspi020.c >> @@ -0,0 +1,345 @@ >> +/* >> + * Faraday FTSPI020 Flash Controller >> + * >> + * Copyright (c) 2012 Faraday Technology >> + * Written by Dante Su <dantesu@faraday-tech.com> >> + * >> + * This code is licensed under GNU GPL v2+. >> + */ >> + >> +#include <hw/hw.h> >> +#include <sysemu/sysemu.h> >> +#include <hw/sysbus.h> >> +#include <hw/ssi.h> >> + >> +#include "ftspi020.h" >> + >> +#define TYPE_FTSPI020 "ftspi020" >> + >> +typedef struct Ftspi020State { >> + SysBusDevice busdev; >> + MemoryRegion iomem; >> + qemu_irq irq; >> + >> + /* DMA hardware handshake */ >> + qemu_irq req; >> + >> + SSIBus *spi; >> + uint8_t num_cs; > > Constant. No need for it to be part of the device state, unless you > want to drive it from a property and let the machine model decide how > my cs lines you have? > Got you! I'll move these stuff to the top of faraday_a36x.c as defines. >> + qemu_irq *cs_lines; >> + >> + int wip; /* SPI Flash Status: Write In Progress BIT shift */ >> + uint32_t datacnt; >> + >> + /* HW register caches */ >> + uint32_t cmd[4]; >> + uint32_t ctrl; >> + uint32_t timing; >> + uint32_t icr; >> + uint32_t isr; >> + uint32_t rdsr; >> +} Ftspi020State; >> + >> +#define FTSPI020(obj) \ >> + OBJECT_CHECK(Ftspi020State, obj, TYPE_FTSPI020) >> + >> +static void ftspi020_update_irq(Ftspi020State *s) >> +{ >> + if (s->isr) { >> + qemu_set_irq(s->irq, 1); >> + } else { >> + qemu_set_irq(s->irq, 0); >> + } >> +} >> + >> +static void ftspi020_handle_ack(void *opaque, int line, int level) >> +{ >> + Ftspi020State *s = FTSPI020(opaque); >> + >> + if (!(s->icr & 0x01)) { >> + return; >> + } >> + >> + if (level) { >> + qemu_set_irq(s->req, 0); >> + } else if (s->datacnt > 0) { >> + qemu_set_irq(s->req, 1); >> + } >> +} >> + >> +static int ftspi020_do_command(Ftspi020State *s) >> +{ >> + int cs = (s->cmd[3] >> 8) & 0x03; >> + int cmd = (s->cmd[3] >> 24) & 0xff; >> + int ilen = (s->cmd[1] >> 24) & 0x03; >> + int alen = (s->cmd[1] >> 0) & 0x07; >> + int dcyc = (s->cmd[1] >> 16) & 0xff; >> + > > bitops.h has helpers that can extract fields for you without all the >>> & stuff. Lots of magic numbers here as well, can you #define these > offsets with meaningful names? Got you! I'll add some BIT OFFSET and MACROs for these stuff. > >> + /* make sure the spi flash is de-activated */ >> + qemu_set_irq(s->cs_lines[cs], 1); >> + >> + /* activate the spi flash */ >> + qemu_set_irq(s->cs_lines[cs], 0); >> + >> + /* if it's a SPI flash READ_STATUS command */ >> + if ((s->cmd[3] & 0x06) == 0x04) { >> + do { >> + ssi_transfer(s->spi, cmd); >> + s->rdsr = ssi_transfer(s->spi, 0x00); >> + if (s->cmd[3] & 0x08) { >> + break; >> + } >> + } while (s->rdsr & (1 << s->wip)); >> + } else { >> + /* otherwise */ >> + int i; >> + >> + ilen = MIN(ilen, 2); >> + alen = MIN(alen, 4); >> + >> + /* command cycles */ >> + for (i = 0; i < ilen; ++i) { >> + ssi_transfer(s->spi, cmd); >> + } >> + /* address cycles */ >> + for (i = alen - 1; i >= 0; --i) { >> + ssi_transfer(s->spi, (s->cmd[0] >> (8 * i)) & 0xff); >> + } >> + /* dummy cycles */ >> + for (i = 0; i < (dcyc >> 3); ++i) { >> + ssi_transfer(s->spi, 0x00); >> + } >> + } >> + >> + if (s->datacnt <= 0) { > > == as this is unsigned. Got you! > >> + qemu_set_irq(s->cs_lines[cs], 1); >> + } else if (s->icr & 0x01) { >> + qemu_set_irq(s->req, 1); >> + } >> + >> + if (s->cmd[3] & 0x01) { >> + s->isr |= 0x01; >> + } >> + ftspi020_update_irq(s); >> + >> + return 0; >> +} >> + >> +static void ftspi020_chip_reset(Ftspi020State *s) >> +{ >> + int i; >> + >> + s->datacnt = 0; >> + for (i = 0; i < 4; ++i) { >> + s->cmd[i] = 0; >> + } >> + s->wip = 0; >> + s->ctrl = 0; >> + s->timing = 0; >> + s->icr = 0; >> + s->isr = 0; >> + s->rdsr = 0; >> + >> + qemu_set_irq(s->irq, 0); > > Do the cs lines need resetting here? > Absolutely >> +} >> + >> +static uint64_t ftspi020_mem_read(void *opaque, hwaddr addr, unsigned size) >> +{ >> + Ftspi020State *s = FTSPI020(opaque); >> + uint64_t ret = 0; >> + >> + switch (addr) { >> + case REG_DATA: >> + if (!(s->cmd[3] & 0x02)) { >> + if (s->datacnt > 0) { >> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 0; >> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 8; >> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 16; >> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 24; > > loopable: > > for (i = 0; i < 4; ++i) { > ret = deposit32(ret, i * 8, 8, (uint32_t)(ssi_transfer(s->spi, > 0x00) & 0xff)); > } > Got you >> + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); >> + } >> + if (s->datacnt == 0) { >> + uint8_t cs = (s->cmd[3] >> 8) & 0x03; >> + qemu_set_irq(s->cs_lines[cs], 1); >> + if (s->cmd[3] & 0x01) { >> + s->isr |= 0x01; >> + } >> + ftspi020_update_irq(s); >> + } >> + } >> + break; >> + case REG_RDST: >> + return s->rdsr; >> + case REG_CMD0: > > the case .. syntax would allow you to collapse these 4 into 1: > > case REG_CMD0 .. REG_CMD3: > return s->cmd[(addr - REG_CMD0) / 4] > Got you >> + return s->cmd[0]; >> + case REG_CMD1: >> + return s->cmd[1]; >> + case REG_CMD2: >> + return s->cmd[2]; >> + case REG_CMD3: >> + return s->cmd[3]; >> + case REG_STR: >> + /* In QEMU, the data fifo is always ready for read/write */ >> + return 0x00000003; >> + case REG_ISR: >> + return s->isr; >> + case REG_ICR: >> + return s->icr; >> + case REG_CTRL: >> + return s->ctrl; >> + case REG_ACT: >> + return s->timing; >> + case REG_REV: >> + return 0x00010001; >> + case REG_FEA: >> + return 0x02022020; > > Few magic numbers here > case REG_REV: return 0x00010001; /* revision 1.0.1 */ case REG_FEA: return 0x02022020; /* Clock Mode=SYNC, cmd queue = 4, tx/rx fifo=32 */ However in QEMU, there is no I/O delay to access the underlying SPI flash, so the clock, and fifo depth information are totally useless. >> + default: >> + break; >> + } >> + >> + return ret; >> +} >> + >> +static void ftspi020_mem_write(void *opaque, >> + hwaddr addr, >> + uint64_t val, >> + unsigned size) >> +{ >> + Ftspi020State *s = FTSPI020(opaque); >> + >> + switch (addr) { >> + case REG_DATA: >> + if (s->cmd[3] & 0x02) { >> + if (s->datacnt > 0) { >> + ssi_transfer(s->spi, (uint8_t)((val >> 0) & 0xff)); >> + ssi_transfer(s->spi, (uint8_t)((val >> 8) & 0xff)); >> + ssi_transfer(s->spi, (uint8_t)((val >> 16) & 0xff)); >> + ssi_transfer(s->spi, (uint8_t)((val >> 24) & 0xff)); >> + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); >> + } >> + if (s->datacnt == 0) { >> + uint8_t cs = (s->cmd[3] >> 8) & 0x03; >> + qemu_set_irq(s->cs_lines[cs], 1); >> + if (s->cmd[3] & 0x01) { >> + s->isr |= 0x01; >> + } >> + ftspi020_update_irq(s); >> + } >> + } >> + break; >> + case REG_CMD0: >> + s->cmd[0] = (uint32_t)val; >> + break; >> + case REG_CMD1: >> + s->cmd[1] = (uint32_t)val; >> + break; >> + case REG_CMD2: >> + s->datacnt = s->cmd[2] = (uint32_t)val; >> + break; >> + case REG_CMD3: >> + s->cmd[3] = (uint32_t)val; >> + ftspi020_do_command(s); >> + break; >> + case REG_ISR: >> + s->isr &= ~((uint32_t)val); >> + ftspi020_update_irq(s); >> + break; >> + case REG_ICR: >> + s->icr = (uint32_t)val; >> + break; >> + case REG_CTRL: >> + if (val & 0x100) { >> + ftspi020_chip_reset(s); >> + } >> + s->ctrl = (uint32_t)val & 0x70013; >> + s->wip = ((uint32_t)val >> 16) & 0x07; >> + break; >> + case REG_ACT: >> + s->timing = (uint32_t)val; >> + break; >> + default: >> + break; >> + } >> +} >> + >> +static const MemoryRegionOps ftspi020_ops = { >> + .read = ftspi020_mem_read, >> + .write = ftspi020_mem_write, >> + .endianness = DEVICE_LITTLE_ENDIAN, >> +}; >> + >> +static void ftspi020_reset(DeviceState *ds) >> +{ >> + SysBusDevice *busdev = SYS_BUS_DEVICE(ds); >> + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, busdev)); >> + >> + ftspi020_chip_reset(s); >> +} >> + >> +static int ftspi020_init(SysBusDevice *dev) >> +{ >> + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, dev)); >> + int i; >> + >> + memory_region_init_io(&s->iomem, >> + &ftspi020_ops, >> + s, >> + TYPE_FTSPI020, >> + 0x1000); >> + sysbus_init_mmio(dev, &s->iomem); >> + sysbus_init_irq(dev, &s->irq); >> + >> + s->spi = ssi_create_bus(&dev->qdev, "spi"); >> + s->num_cs = 4; >> + s->cs_lines = g_new(qemu_irq, s->num_cs); >> + ssi_auto_connect_slaves(DEVICE(s), s->cs_lines, s->spi); >> + for (i = 0; i < s->num_cs; ++i) { >> + sysbus_init_irq(dev, &s->cs_lines[i]); >> + } >> + >> + qdev_init_gpio_in(&s->busdev.qdev, ftspi020_handle_ack, 1); >> + qdev_init_gpio_out(&s->busdev.qdev, &s->req, 1); >> + >> + return 0; >> +} >> + >> +static const VMStateDescription vmstate_ftspi020 = { >> + .name = TYPE_FTSPI020, >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .minimum_version_id_old = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT32_ARRAY(cmd, Ftspi020State, 4), >> + VMSTATE_UINT32(ctrl, Ftspi020State), >> + VMSTATE_UINT32(timing, Ftspi020State), >> + VMSTATE_UINT32(icr, Ftspi020State), >> + VMSTATE_UINT32(isr, Ftspi020State), >> + VMSTATE_UINT32(rdsr, Ftspi020State), > > datacnt is missing. I think you need to save it as it is device state > that would need to be preserved if you machine got migrated in the > middle of a transaction. > Got you >> + VMSTATE_END_OF_LIST(), >> + } >> +}; >> + >> +static void ftspi020_class_init(ObjectClass *klass, void *data) >> +{ >> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + k->init = ftspi020_init; >> + dc->vmsd = &vmstate_ftspi020; >> + dc->reset = ftspi020_reset; >> + dc->no_user = 1; >> +} >> + >> +static const TypeInfo ftspi020_info = { >> + .name = TYPE_FTSPI020, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(Ftspi020State), >> + .class_init = ftspi020_class_init, >> +}; >> + >> +static void ftspi020_register_types(void) >> +{ >> + type_register_static(&ftspi020_info); >> +} >> + >> +type_init(ftspi020_register_types) >> diff --git a/hw/arm/ftspi020.h b/hw/arm/ftspi020.h >> new file mode 100644 >> index 0000000..a8a0930 >> --- /dev/null >> +++ b/hw/arm/ftspi020.h >> @@ -0,0 +1,50 @@ >> +/* >> + * Faraday FTSPI020 Flash Controller >> + * >> + * Copyright (c) 2012 Faraday Technology >> + * Written by Dante Su <dantesu@faraday-tech.com> >> + * >> + * This code is licensed under GNU GPL v2+. >> + */ >> + >> +#ifndef HW_ARM_FTSPI020_H >> +#define HW_ARM_FTSPI020_H >> + > > This device is not exporting any extensible APIs there is no need for > a header specific to this device. > Got you, but does this applied to ftrtc011 ? I remember someone had made a request to me to create header files for registers. Or maybe it's just a mis-understanding. >> +/****************************************************************************** >> + * FTSPI020 registers >> + *****************************************************************************/ >> +#define REG_CMD0 0x00 /* Flash address */ >> +#define REG_CMD1 0x04 >> +#define REG_CMD2 0x08 >> +#define REG_CMD3 0x0c >> +#define REG_CTRL 0x10 /* Control Register */ >> +#define REG_ACT 0x14 /* AC Timing Register */ >> +#define REG_STR 0x18 /* Status Register */ >> +#define REG_ICR 0x20 /* Interrupt Control Register */ >> +#define REG_ISR 0x24 /* Interrupt Status Register */ >> +#define REG_RDST 0x28 /* Read Status Register */ >> +#define REG_REV 0x50 /* Revision Register */ >> +#define REG_FEA 0x54 /* Feature Register */ >> +#define REG_DATA 0x100 /* Data Register */ >> + > > These can just live in the C file - they are private to the implementation. > Same above >> +/****************************************************************************** >> + * Common SPI flash opcodes (Not FTSPI020 specific) >> + *****************************************************************************/ >> +#define OPCODE_WREN 0x06 /* Write enable */ >> +#define OPCODE_WRSR 0x01 /* Write status register 1 byte */ >> +#define OPCODE_RDSR 0x05 /* Read status register */ >> +#define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ >> +#define OPCODE_NORM_READ4 0x13 /* Read data bytes (4 bytes address) */ >> +#define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ >> +#define OPCODE_FAST_READ4 0x0c /* Read data bytes (4 bytes address) */ >> +#define OPCODE_PP 0x02 /* Page program (up to 256 bytes) */ >> +#define OPCODE_PP4 0x12 /* Page program (4 bytes address) */ >> +#define OPCODE_SE 0xd8 /* Sector erase (usually 64KiB) */ >> +#define OPCODE_SE4 0xdc /* Sector erase (4 bytes address) */ >> +#define OPCODE_RDID 0x9f /* Read JEDEC ID */ >> + > > This is repetition of M25P80s command opcodes. If anything, we should > factor these out into m25p80.h (doesnt exist yet) as this is the > second device model (apart from m25p80 itself) to need these. The > other one is xilinx_spips.c. However i notice almost all of these are > unused by your device model. Apart from the little bit of logic around > RDSR, is there any flash command-specific functionality in this > device? AFAICT it just accepts command code, address, num dummies and > payload size and the details of what those commands mean is abstracted > away from this hardware, making this table mostly redundant. If you > have future work that will need this that's another matter however. > You're absolutely right, the SPI flash commands has been abstracted. None of the OPCODE is used i the model. About the RDSR, it requires the driver to set corresponding BIT in s->cmd[3] before issuing a RDSR(0x05) to underlying SPI flash device. These bits are BIT3 - 0: polling status by hardware, driver should polling the status register of FTSPI020 to check when the flash is back to idle state. (RDSR is sent only once) 1: polling status by hardware, driver would have to keep sending RDSR and read the status back to check if the flash is now idle. BIT2 - 0: this command is not a RDSR (READ_STATUS) 1: this command is a RDSR (READ_STATUS) BTW, tomorrow is the beginning of my Chinese New Year holidays, so please forgive me that I won't be able to make any response to the comments for 10 days. > Regards, > Peter > >> +/* Status Register bits. */ >> +#define SR_WIP 1 /* Write in progress */ >> +#define SR_WEL 2 /* Write enable latch */ >> + >> +#endif >> -- >> 1.7.9.5 >> >> -- Best wishes, Kuo-Jung Su
On Thu, Feb 7, 2013 at 4:59 PM, Kuo-Jung Su <dantesu@gmail.com> wrote: > \ > > 2013/2/7 Peter Crosthwaite <peter.crosthwaite@xilinx.com>: >> Hi Kuo-Jung, >> >> On Wed, Feb 6, 2013 at 7:45 PM, Kuo-Jung Su <dantesu@gmail.com> wrote: >>> From: Kuo-Jung Su <dantesu@faraday-tech.com> >>> >>> The FTSPI020 is an integrated SPI Flash controller >>> which supports upto 4 flash chips. >>> >>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com> >>> --- >>> hw/arm/Makefile.objs | 1 + >>> hw/arm/faraday_a369.c | 12 ++ >>> hw/arm/ftspi020.c | 345 +++++++++++++++++++++++++++++++++++++++++++++++++ >>> hw/arm/ftspi020.h | 50 +++++++ >>> 4 files changed, 408 insertions(+) >>> create mode 100644 hw/arm/ftspi020.c >>> create mode 100644 hw/arm/ftspi020.h >>> >>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs >>> index 508335a..7178b5d 100644 >>> --- a/hw/arm/Makefile.objs >>> +++ b/hw/arm/Makefile.objs >>> @@ -52,3 +52,4 @@ obj-y += ftgmac100.o >>> obj-y += ftlcdc200.o >>> obj-y += fttsc010.o >>> obj-y += ftsdc010.o >>> +obj-y += ftspi020.o >>> diff --git a/hw/arm/faraday_a369.c b/hw/arm/faraday_a369.c >>> index 4ad48f5..132ee2f 100644 >>> --- a/hw/arm/faraday_a369.c >>> +++ b/hw/arm/faraday_a369.c >>> @@ -203,6 +203,18 @@ a369_device_init(A369State *s) >>> req = qdev_get_gpio_in(s->hdma[0], 13); >>> qdev_connect_gpio_out(s->hdma[0], 13, ack); >>> qdev_connect_gpio_out(ds, 0, req); >>> + >>> + /* ftspi020: as an external AHB device */ >>> + ds = sysbus_create_simple("ftspi020", 0xC0000000, pic[13]); >>> + spi = (SSIBus *)qdev_get_child_bus(ds, "spi"); >>> + nr_flash = 1; >>> + for (i = 0; i < nr_flash; i++) { >>> + fl = ssi_create_slave_no_init(spi, "m25p80"); >>> + qdev_prop_set_string(fl, "partname", "w25q64"); >>> + qdev_init_nofail(fl); >>> + cs_line = qdev_get_gpio_in(fl, 0); >>> + sysbus_connect_irq(SYS_BUS_DEVICE(ds), i + 1, cs_line); >>> + } >>> } >>> >>> static void >>> diff --git a/hw/arm/ftspi020.c b/hw/arm/ftspi020.c >>> new file mode 100644 >>> index 0000000..dab2f6f >>> --- /dev/null >>> +++ b/hw/arm/ftspi020.c >>> @@ -0,0 +1,345 @@ >>> +/* >>> + * Faraday FTSPI020 Flash Controller >>> + * >>> + * Copyright (c) 2012 Faraday Technology >>> + * Written by Dante Su <dantesu@faraday-tech.com> >>> + * >>> + * This code is licensed under GNU GPL v2+. >>> + */ >>> + >>> +#include <hw/hw.h> >>> +#include <sysemu/sysemu.h> >>> +#include <hw/sysbus.h> >>> +#include <hw/ssi.h> >>> + >>> +#include "ftspi020.h" >>> + >>> +#define TYPE_FTSPI020 "ftspi020" >>> + >>> +typedef struct Ftspi020State { >>> + SysBusDevice busdev; >>> + MemoryRegion iomem; >>> + qemu_irq irq; >>> + >>> + /* DMA hardware handshake */ >>> + qemu_irq req; >>> + >>> + SSIBus *spi; >>> + uint8_t num_cs; >> >> Constant. No need for it to be part of the device state, unless you >> want to drive it from a property and let the machine model decide how >> my cs lines you have? >> > > Got you! I'll move these stuff to the top of faraday_a36x.c as defines. > >>> + qemu_irq *cs_lines; >>> + >>> + int wip; /* SPI Flash Status: Write In Progress BIT shift */ >>> + uint32_t datacnt; >>> + >>> + /* HW register caches */ >>> + uint32_t cmd[4]; >>> + uint32_t ctrl; >>> + uint32_t timing; >>> + uint32_t icr; >>> + uint32_t isr; >>> + uint32_t rdsr; >>> +} Ftspi020State; >>> + >>> +#define FTSPI020(obj) \ >>> + OBJECT_CHECK(Ftspi020State, obj, TYPE_FTSPI020) >>> + >>> +static void ftspi020_update_irq(Ftspi020State *s) >>> +{ >>> + if (s->isr) { >>> + qemu_set_irq(s->irq, 1); >>> + } else { >>> + qemu_set_irq(s->irq, 0); >>> + } >>> +} >>> + >>> +static void ftspi020_handle_ack(void *opaque, int line, int level) >>> +{ >>> + Ftspi020State *s = FTSPI020(opaque); >>> + >>> + if (!(s->icr & 0x01)) { >>> + return; >>> + } >>> + >>> + if (level) { >>> + qemu_set_irq(s->req, 0); >>> + } else if (s->datacnt > 0) { >>> + qemu_set_irq(s->req, 1); >>> + } >>> +} >>> + >>> +static int ftspi020_do_command(Ftspi020State *s) >>> +{ >>> + int cs = (s->cmd[3] >> 8) & 0x03; >>> + int cmd = (s->cmd[3] >> 24) & 0xff; >>> + int ilen = (s->cmd[1] >> 24) & 0x03; >>> + int alen = (s->cmd[1] >> 0) & 0x07; >>> + int dcyc = (s->cmd[1] >> 16) & 0xff; >>> + >> >> bitops.h has helpers that can extract fields for you without all the >>>> & stuff. Lots of magic numbers here as well, can you #define these >> offsets with meaningful names? > > Got you! I'll add some BIT OFFSET and MACROs for these stuff. > >> >>> + /* make sure the spi flash is de-activated */ >>> + qemu_set_irq(s->cs_lines[cs], 1); >>> + >>> + /* activate the spi flash */ >>> + qemu_set_irq(s->cs_lines[cs], 0); >>> + >>> + /* if it's a SPI flash READ_STATUS command */ >>> + if ((s->cmd[3] & 0x06) == 0x04) { >>> + do { >>> + ssi_transfer(s->spi, cmd); >>> + s->rdsr = ssi_transfer(s->spi, 0x00); >>> + if (s->cmd[3] & 0x08) { >>> + break; >>> + } >>> + } while (s->rdsr & (1 << s->wip)); >>> + } else { >>> + /* otherwise */ >>> + int i; >>> + >>> + ilen = MIN(ilen, 2); >>> + alen = MIN(alen, 4); >>> + >>> + /* command cycles */ >>> + for (i = 0; i < ilen; ++i) { >>> + ssi_transfer(s->spi, cmd); >>> + } >>> + /* address cycles */ >>> + for (i = alen - 1; i >= 0; --i) { >>> + ssi_transfer(s->spi, (s->cmd[0] >> (8 * i)) & 0xff); >>> + } >>> + /* dummy cycles */ >>> + for (i = 0; i < (dcyc >> 3); ++i) { >>> + ssi_transfer(s->spi, 0x00); >>> + } >>> + } >>> + >>> + if (s->datacnt <= 0) { >> >> == as this is unsigned. > > Got you! > On second thoughts, just use if (!s->datacnt) { dont need ==. This is fairly standard for testing a down-to-0 counter. >> >>> + qemu_set_irq(s->cs_lines[cs], 1); >>> + } else if (s->icr & 0x01) { >>> + qemu_set_irq(s->req, 1); >>> + } >>> + >>> + if (s->cmd[3] & 0x01) { >>> + s->isr |= 0x01; >>> + } >>> + ftspi020_update_irq(s); >>> + >>> + return 0; >>> +} >>> + >>> +static void ftspi020_chip_reset(Ftspi020State *s) >>> +{ >>> + int i; >>> + >>> + s->datacnt = 0; >>> + for (i = 0; i < 4; ++i) { >>> + s->cmd[i] = 0; >>> + } >>> + s->wip = 0; >>> + s->ctrl = 0; >>> + s->timing = 0; >>> + s->icr = 0; >>> + s->isr = 0; >>> + s->rdsr = 0; >>> + >>> + qemu_set_irq(s->irq, 0); >> >> Do the cs lines need resetting here? >> > > Absolutely > >>> +} >>> + >>> +static uint64_t ftspi020_mem_read(void *opaque, hwaddr addr, unsigned size) >>> +{ >>> + Ftspi020State *s = FTSPI020(opaque); >>> + uint64_t ret = 0; >>> + >>> + switch (addr) { >>> + case REG_DATA: >>> + if (!(s->cmd[3] & 0x02)) { >>> + if (s->datacnt > 0) { >>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 0; >>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 8; >>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 16; >>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 24; >> >> loopable: >> >> for (i = 0; i < 4; ++i) { >> ret = deposit32(ret, i * 8, 8, (uint32_t)(ssi_transfer(s->spi, >> 0x00) & 0xff)); >> } >> > > Got you > >>> + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); >>> + } >>> + if (s->datacnt == 0) { >>> + uint8_t cs = (s->cmd[3] >> 8) & 0x03; >>> + qemu_set_irq(s->cs_lines[cs], 1); >>> + if (s->cmd[3] & 0x01) { >>> + s->isr |= 0x01; >>> + } >>> + ftspi020_update_irq(s); >>> + } >>> + } >>> + break; >>> + case REG_RDST: >>> + return s->rdsr; >>> + case REG_CMD0: >> >> the case .. syntax would allow you to collapse these 4 into 1: >> >> case REG_CMD0 .. REG_CMD3: >> return s->cmd[(addr - REG_CMD0) / 4] >> > > Got you > >>> + return s->cmd[0]; >>> + case REG_CMD1: >>> + return s->cmd[1]; >>> + case REG_CMD2: >>> + return s->cmd[2]; >>> + case REG_CMD3: >>> + return s->cmd[3]; >>> + case REG_STR: >>> + /* In QEMU, the data fifo is always ready for read/write */ >>> + return 0x00000003; >>> + case REG_ISR: >>> + return s->isr; >>> + case REG_ICR: >>> + return s->icr; >>> + case REG_CTRL: >>> + return s->ctrl; >>> + case REG_ACT: >>> + return s->timing; >>> + case REG_REV: >>> + return 0x00010001; >>> + case REG_FEA: >>> + return 0x02022020; >> >> Few magic numbers here >> > > case REG_REV: > return 0x00010001; /* revision 1.0.1 */ ACK > case REG_FEA: > return 0x02022020; /* Clock Mode=SYNC, cmd queue = 4, tx/rx fifo=32 */ > You could these as three pieces as macros and | them together. I wont block on it however. > However in QEMU, there is no I/O delay to access the underlying SPI flash, > so the clock, and fifo depth information are totally useless. > >>> + default: >>> + break; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +static void ftspi020_mem_write(void *opaque, >>> + hwaddr addr, >>> + uint64_t val, >>> + unsigned size) >>> +{ >>> + Ftspi020State *s = FTSPI020(opaque); >>> + >>> + switch (addr) { >>> + case REG_DATA: >>> + if (s->cmd[3] & 0x02) { >>> + if (s->datacnt > 0) { >>> + ssi_transfer(s->spi, (uint8_t)((val >> 0) & 0xff)); >>> + ssi_transfer(s->spi, (uint8_t)((val >> 8) & 0xff)); >>> + ssi_transfer(s->spi, (uint8_t)((val >> 16) & 0xff)); >>> + ssi_transfer(s->spi, (uint8_t)((val >> 24) & 0xff)); >>> + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); >>> + } >>> + if (s->datacnt == 0) { >>> + uint8_t cs = (s->cmd[3] >> 8) & 0x03; >>> + qemu_set_irq(s->cs_lines[cs], 1); >>> + if (s->cmd[3] & 0x01) { >>> + s->isr |= 0x01; >>> + } >>> + ftspi020_update_irq(s); >>> + } >>> + } >>> + break; >>> + case REG_CMD0: >>> + s->cmd[0] = (uint32_t)val; >>> + break; >>> + case REG_CMD1: >>> + s->cmd[1] = (uint32_t)val; >>> + break; >>> + case REG_CMD2: >>> + s->datacnt = s->cmd[2] = (uint32_t)val; >>> + break; >>> + case REG_CMD3: >>> + s->cmd[3] = (uint32_t)val; >>> + ftspi020_do_command(s); >>> + break; >>> + case REG_ISR: >>> + s->isr &= ~((uint32_t)val); >>> + ftspi020_update_irq(s); >>> + break; >>> + case REG_ICR: >>> + s->icr = (uint32_t)val; >>> + break; >>> + case REG_CTRL: >>> + if (val & 0x100) { >>> + ftspi020_chip_reset(s); >>> + } >>> + s->ctrl = (uint32_t)val & 0x70013; >>> + s->wip = ((uint32_t)val >> 16) & 0x07; >>> + break; >>> + case REG_ACT: >>> + s->timing = (uint32_t)val; >>> + break; >>> + default: >>> + break; >>> + } >>> +} >>> + >>> +static const MemoryRegionOps ftspi020_ops = { >>> + .read = ftspi020_mem_read, >>> + .write = ftspi020_mem_write, >>> + .endianness = DEVICE_LITTLE_ENDIAN, >>> +}; >>> + >>> +static void ftspi020_reset(DeviceState *ds) >>> +{ >>> + SysBusDevice *busdev = SYS_BUS_DEVICE(ds); >>> + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, busdev)); >>> + >>> + ftspi020_chip_reset(s); >>> +} >>> + >>> +static int ftspi020_init(SysBusDevice *dev) >>> +{ >>> + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, dev)); >>> + int i; >>> + >>> + memory_region_init_io(&s->iomem, >>> + &ftspi020_ops, >>> + s, >>> + TYPE_FTSPI020, >>> + 0x1000); >>> + sysbus_init_mmio(dev, &s->iomem); >>> + sysbus_init_irq(dev, &s->irq); >>> + >>> + s->spi = ssi_create_bus(&dev->qdev, "spi"); >>> + s->num_cs = 4; >>> + s->cs_lines = g_new(qemu_irq, s->num_cs); >>> + ssi_auto_connect_slaves(DEVICE(s), s->cs_lines, s->spi); >>> + for (i = 0; i < s->num_cs; ++i) { >>> + sysbus_init_irq(dev, &s->cs_lines[i]); >>> + } >>> + >>> + qdev_init_gpio_in(&s->busdev.qdev, ftspi020_handle_ack, 1); >>> + qdev_init_gpio_out(&s->busdev.qdev, &s->req, 1); >>> + >>> + return 0; >>> +} >>> + >>> +static const VMStateDescription vmstate_ftspi020 = { >>> + .name = TYPE_FTSPI020, >>> + .version_id = 1, >>> + .minimum_version_id = 1, >>> + .minimum_version_id_old = 1, >>> + .fields = (VMStateField[]) { >>> + VMSTATE_UINT32_ARRAY(cmd, Ftspi020State, 4), >>> + VMSTATE_UINT32(ctrl, Ftspi020State), >>> + VMSTATE_UINT32(timing, Ftspi020State), >>> + VMSTATE_UINT32(icr, Ftspi020State), >>> + VMSTATE_UINT32(isr, Ftspi020State), >>> + VMSTATE_UINT32(rdsr, Ftspi020State), >> >> datacnt is missing. I think you need to save it as it is device state >> that would need to be preserved if you machine got migrated in the >> middle of a transaction. >> > > Got you > >>> + VMSTATE_END_OF_LIST(), >>> + } >>> +}; >>> + >>> +static void ftspi020_class_init(ObjectClass *klass, void *data) >>> +{ >>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + >>> + k->init = ftspi020_init; >>> + dc->vmsd = &vmstate_ftspi020; >>> + dc->reset = ftspi020_reset; >>> + dc->no_user = 1; >>> +} >>> + >>> +static const TypeInfo ftspi020_info = { >>> + .name = TYPE_FTSPI020, >>> + .parent = TYPE_SYS_BUS_DEVICE, >>> + .instance_size = sizeof(Ftspi020State), >>> + .class_init = ftspi020_class_init, >>> +}; >>> + >>> +static void ftspi020_register_types(void) >>> +{ >>> + type_register_static(&ftspi020_info); >>> +} >>> + >>> +type_init(ftspi020_register_types) >>> diff --git a/hw/arm/ftspi020.h b/hw/arm/ftspi020.h >>> new file mode 100644 >>> index 0000000..a8a0930 >>> --- /dev/null >>> +++ b/hw/arm/ftspi020.h >>> @@ -0,0 +1,50 @@ >>> +/* >>> + * Faraday FTSPI020 Flash Controller >>> + * >>> + * Copyright (c) 2012 Faraday Technology >>> + * Written by Dante Su <dantesu@faraday-tech.com> >>> + * >>> + * This code is licensed under GNU GPL v2+. >>> + */ >>> + >>> +#ifndef HW_ARM_FTSPI020_H >>> +#define HW_ARM_FTSPI020_H >>> + >> >> This device is not exporting any extensible APIs there is no need for >> a header specific to this device. >> > > Got you, but does this applied to ftrtc011 ? > I remember someone had made a request to me to create header files > for registers. Or maybe it's just a mis-understanding. > Can you do me a fav and link me the discussion? >>> +/****************************************************************************** >>> + * FTSPI020 registers >>> + *****************************************************************************/ >>> +#define REG_CMD0 0x00 /* Flash address */ >>> +#define REG_CMD1 0x04 >>> +#define REG_CMD2 0x08 >>> +#define REG_CMD3 0x0c >>> +#define REG_CTRL 0x10 /* Control Register */ >>> +#define REG_ACT 0x14 /* AC Timing Register */ >>> +#define REG_STR 0x18 /* Status Register */ >>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>> +#define REG_RDST 0x28 /* Read Status Register */ >>> +#define REG_REV 0x50 /* Revision Register */ >>> +#define REG_FEA 0x54 /* Feature Register */ >>> +#define REG_DATA 0x100 /* Data Register */ >>> + >> >> These can just live in the C file - they are private to the implementation. >> > > Same above > >>> +/****************************************************************************** >>> + * Common SPI flash opcodes (Not FTSPI020 specific) >>> + *****************************************************************************/ >>> +#define OPCODE_WREN 0x06 /* Write enable */ >>> +#define OPCODE_WRSR 0x01 /* Write status register 1 byte */ >>> +#define OPCODE_RDSR 0x05 /* Read status register */ >>> +#define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ >>> +#define OPCODE_NORM_READ4 0x13 /* Read data bytes (4 bytes address) */ >>> +#define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ >>> +#define OPCODE_FAST_READ4 0x0c /* Read data bytes (4 bytes address) */ >>> +#define OPCODE_PP 0x02 /* Page program (up to 256 bytes) */ >>> +#define OPCODE_PP4 0x12 /* Page program (4 bytes address) */ >>> +#define OPCODE_SE 0xd8 /* Sector erase (usually 64KiB) */ >>> +#define OPCODE_SE4 0xdc /* Sector erase (4 bytes address) */ >>> +#define OPCODE_RDID 0x9f /* Read JEDEC ID */ >>> + >> >> This is repetition of M25P80s command opcodes. If anything, we should >> factor these out into m25p80.h (doesnt exist yet) as this is the >> second device model (apart from m25p80 itself) to need these. The >> other one is xilinx_spips.c. However i notice almost all of these are >> unused by your device model. Apart from the little bit of logic around >> RDSR, is there any flash command-specific functionality in this >> device? AFAICT it just accepts command code, address, num dummies and >> payload size and the details of what those commands mean is abstracted >> away from this hardware, making this table mostly redundant. If you >> have future work that will need this that's another matter however. >> > > You're absolutely right, the SPI flash commands has been abstracted. > None of the OPCODE is used i the model. > Then i think we delete the table in its entirety. The RDSR part can be local to the C file (if needed see comment below). > About the RDSR, it requires the driver to set corresponding BIT in s->cmd[3] > before issuing a RDSR(0x05) to underlying SPI flash device. > Currently your hardware (as I read it) is snooping the requested command to changing behavior based on if (cmd = RDSR). From what you say here however, isnt this alternate behaviour triggered by a s->cmd[3] bit field? I.e. can the correct bevhaviour of this device be captured by your device watching s->cmd[3] rather than checking the request command for the RDSR opcode? If this is the case then just delete the whole table and drive your logic off your command registers. > These bits are > > BIT3 - 0: polling status by hardware, driver should polling the status > register of FTSPI020 > to check when the flash is back to idle state. (RDSR is > sent only once) > 1: polling status by hardware, driver would have to keep > sending RDSR and > read the status back to check if the flash is now idle. > > BIT2 - 0: this command is not a RDSR (READ_STATUS) > 1: this command is a RDSR (READ_STATUS) > > BTW, tomorrow is the beginning of my Chinese New Year holidays, so > please forgive me > that I won't be able to make any response to the comments for 10 days. > No rush. Enjoy your holidays. Regards, Peter
Am 07.02.2013 07:59, schrieb Kuo-Jung Su: > 2013/2/7 Peter Crosthwaite <peter.crosthwaite@xilinx.com>: >> On Wed, Feb 6, 2013 at 7:45 PM, Kuo-Jung Su <dantesu@gmail.com> wrote: >>> diff --git a/hw/arm/ftspi020.h b/hw/arm/ftspi020.h >>> new file mode 100644 >>> index 0000000..a8a0930 >>> --- /dev/null >>> +++ b/hw/arm/ftspi020.h >>> @@ -0,0 +1,50 @@ >>> +/* >>> + * Faraday FTSPI020 Flash Controller >>> + * >>> + * Copyright (c) 2012 Faraday Technology >>> + * Written by Dante Su <dantesu@faraday-tech.com> >>> + * >>> + * This code is licensed under GNU GPL v2+. >>> + */ >>> + >>> +#ifndef HW_ARM_FTSPI020_H >>> +#define HW_ARM_FTSPI020_H >>> + >> >> This device is not exporting any extensible APIs there is no need for >> a header specific to this device. >> > > Got you, but does this applied to ftrtc011 ? > I remember someone had made a request to me to create header files > for registers. Or maybe it's just a mis-understanding. > >>> +/****************************************************************************** >>> + * FTSPI020 registers >>> + *****************************************************************************/ >>> +#define REG_CMD0 0x00 /* Flash address */ >>> +#define REG_CMD1 0x04 >>> +#define REG_CMD2 0x08 >>> +#define REG_CMD3 0x0c >>> +#define REG_CTRL 0x10 /* Control Register */ >>> +#define REG_ACT 0x14 /* AC Timing Register */ >>> +#define REG_STR 0x18 /* Status Register */ >>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>> +#define REG_RDST 0x28 /* Read Status Register */ >>> +#define REG_REV 0x50 /* Revision Register */ >>> +#define REG_FEA 0x54 /* Feature Register */ >>> +#define REG_DATA 0x100 /* Data Register */ >>> + >> >> These can just live in the C file - they are private to the implementation. No. Having them in a header means they can be reused by qtests. To embed the device into a SoC object, the state struct and TYPE_ constant should be in the header. That applies to each device part of the SoC rather than on the board. I did suggest splitting the header into <foo>_regs.h for qtest+softmmu and <foo>.h for softmmu only though. > BTW, tomorrow is the beginning of my Chinese New Year holidays, so > please forgive me > that I won't be able to make any response to the comments for 10 days. v1.4.0 is going to be released Feb 15. Before that no new patches can be applied anyway. http://wiki.qemu.org/Planning/1.4 Regards, Andreas
On 7 February 2013 11:50, Andreas Färber <afaerber@suse.de> wrote: > Am 07.02.2013 07:59, schrieb Kuo-Jung Su: >>>> +/****************************************************************************** >>>> + * FTSPI020 registers >>>> + *****************************************************************************/ >>>> +#define REG_CMD0 0x00 /* Flash address */ >>>> +#define REG_CMD1 0x04 >>>> +#define REG_CMD2 0x08 >>>> +#define REG_CMD3 0x0c >>>> +#define REG_CTRL 0x10 /* Control Register */ >>>> +#define REG_ACT 0x14 /* AC Timing Register */ >>>> +#define REG_STR 0x18 /* Status Register */ >>>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>>> +#define REG_RDST 0x28 /* Read Status Register */ >>>> +#define REG_REV 0x50 /* Revision Register */ >>>> +#define REG_FEA 0x54 /* Feature Register */ >>>> +#define REG_DATA 0x100 /* Data Register */ >>>> + >>> >>> These can just live in the C file - they are private to the implementation. > > No. Having them in a header means they can be reused by qtests. Do we really want to change the public/private boundaries of our source code just for the benefit of the test framework? Ugh. > To embed the device into a SoC object, the state struct and TYPE_ > constant should be in the header. That applies to each device part of > the SoC rather than on the board. I would like it if we could define what we consider to be best practice for public/private header files for QOM devices (in particular whether we need a public header for every device with the TYPE_ constants &c). -- PMM
Am 07.02.2013 13:04, schrieb Peter Maydell: > On 7 February 2013 11:50, Andreas Färber <afaerber@suse.de> wrote: >> Am 07.02.2013 07:59, schrieb Kuo-Jung Su: >>>>> +/****************************************************************************** >>>>> + * FTSPI020 registers >>>>> + *****************************************************************************/ >>>>> +#define REG_CMD0 0x00 /* Flash address */ >>>>> +#define REG_CMD1 0x04 >>>>> +#define REG_CMD2 0x08 >>>>> +#define REG_CMD3 0x0c >>>>> +#define REG_CTRL 0x10 /* Control Register */ >>>>> +#define REG_ACT 0x14 /* AC Timing Register */ >>>>> +#define REG_STR 0x18 /* Status Register */ >>>>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>>>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>>>> +#define REG_RDST 0x28 /* Read Status Register */ >>>>> +#define REG_REV 0x50 /* Revision Register */ >>>>> +#define REG_FEA 0x54 /* Feature Register */ >>>>> +#define REG_DATA 0x100 /* Data Register */ >>>>> + >>>> >>>> These can just live in the C file - they are private to the implementation. >> >> No. Having them in a header means they can be reused by qtests. > > Do we really want to change the public/private boundaries of > our source code just for the benefit of the test framework? Ugh. The truth is (and that applies to the hw/ refactoring series as well) that we don't have a clear distinction between "public" and "private" headers for devices. >> To embed the device into a SoC object, the state struct and TYPE_ >> constant should be in the header. That applies to each device part of >> the SoC rather than on the board. > > I would like it if we could define what we consider to be best > practice for public/private header files for QOM devices > (in particular whether we need a public header for every device > with the TYPE_ constants &c). gtk-doc for instance needs to scan one directory, so having all headers in include/ would be beneficial long-term - today we do not get generated documentation for qdev-core.h. The separation between PCI internal helpers and structs/typedefs-to-be-used is made by including specific headers depending on what you need. In the refcatoring thread I suggested that if we want to have all SPI devices in hw/spi/ then each device that needs a public function declaration or TYPE_ or struct should export its own header. If we group them into a common directory such as hw/arm/ that could be shared across SoC devices. Therefore I was suggesting not to split up too much but seem to be overruled. ;) There's downsides to everything. Andreas
Il 07/02/2013 13:13, Andreas Färber ha scritto: > Am 07.02.2013 13:04, schrieb Peter Maydell: >> On 7 February 2013 11:50, Andreas Färber <afaerber@suse.de> wrote: >>> Am 07.02.2013 07:59, schrieb Kuo-Jung Su: >>>>>> +/****************************************************************************** >>>>>> + * FTSPI020 registers >>>>>> + *****************************************************************************/ >>>>>> +#define REG_CMD0 0x00 /* Flash address */ >>>>>> +#define REG_CMD1 0x04 >>>>>> +#define REG_CMD2 0x08 >>>>>> +#define REG_CMD3 0x0c >>>>>> +#define REG_CTRL 0x10 /* Control Register */ >>>>>> +#define REG_ACT 0x14 /* AC Timing Register */ >>>>>> +#define REG_STR 0x18 /* Status Register */ >>>>>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>>>>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>>>>> +#define REG_RDST 0x28 /* Read Status Register */ >>>>>> +#define REG_REV 0x50 /* Revision Register */ >>>>>> +#define REG_FEA 0x54 /* Feature Register */ >>>>>> +#define REG_DATA 0x100 /* Data Register */ >>>>>> + >>>>> >>>>> These can just live in the C file - they are private to the implementation. >>> >>> No. Having them in a header means they can be reused by qtests. >> >> Do we really want to change the public/private boundaries of >> our source code just for the benefit of the test framework? Ugh. > > The truth is (and that applies to the hw/ refactoring series as well) > that we don't have a clear distinction between "public" and "private" > headers for devices. Both of you are right. Perhaps qtests should live in hw/tests. Not going to do that myself for now. >>> To embed the device into a SoC object, the state struct and TYPE_ >>> constant should be in the header. That applies to each device part of >>> the SoC rather than on the board. >> >> I would like it if we could define what we consider to be best >> practice for public/private header files for QOM devices >> (in particular whether we need a public header for every device >> with the TYPE_ constants &c). > > gtk-doc for instance needs to scan one directory, so having all headers > in include/ would be beneficial long-term - today we do not get > generated documentation for qdev-core.h. > > The separation between PCI internal helpers and > structs/typedefs-to-be-used is made by including specific headers > depending on what you need. > > In the refcatoring thread I suggested that if we want to have all SPI > devices in hw/spi/ then each device that needs a public function > declaration or TYPE_ or struct should export its own header. I think that's not a bad thing. > If we group > them into a common directory such as hw/arm/ that could be shared across > SoC devices. And then you get problems with NEED_CPU_H... :) > Therefore I was suggesting not to split up too much but > seem to be overruled. ;) There's downsides to everything. Yes, that's why I call the current situation a local optimum. Paolo
On 7 February 2013 12:13, Andreas Färber <afaerber@suse.de> wrote: > Am 07.02.2013 13:04, schrieb Peter Maydell: >> Do we really want to change the public/private boundaries of >> our source code just for the benefit of the test framework? Ugh. > > The truth is (and that applies to the hw/ refactoring series as well) > that we don't have a clear distinction between "public" and "private" > headers for devices. Yes. I would like it if we could create one :-) The traditional qdev approach has been (IMHO) that there should be no public header, because to instantiate and use a qdev device you just need its name as a string. Conversion to qdev has often allowed removal of public init functions etc that have been in public-facing header files. QOM seems to want to make a little more public (especially for abstract classes that will be subclassed). It's not clear to me if for QOM qdev devices we should now be preferring qdev_create(NULL, TYPE_WOMBAT) to qdev_create(NULL, "wombat") for straightforward non-abstract device types. I don't have a strong opinion on what we should do, but I would like to see "QOM best practices" clearly documented so that we can point people at it when they're implementing new devices or converting existing ones, to gain some measure of consistency. >> I would like it if we could define what we consider to be best >> practice for public/private header files for QOM devices >> (in particular whether we need a public header for every device >> with the TYPE_ constants &c). > > gtk-doc for instance needs to scan one directory, so having all headers > in include/ would be beneficial long-term - today we do not get > generated documentation for qdev-core.h. This is a bug in gtk-doc and should be fixed there if we care. Also not all doc comments will be in header files; some are in .c files, so you need to scan the whole source tree anyhow. -- PMM
On Thu, Feb 7, 2013 at 10:13 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 07.02.2013 13:04, schrieb Peter Maydell: >> On 7 February 2013 11:50, Andreas Färber <afaerber@suse.de> wrote: >>> Am 07.02.2013 07:59, schrieb Kuo-Jung Su: >>>>>> +/****************************************************************************** >>>>>> + * FTSPI020 registers >>>>>> + *****************************************************************************/ >>>>>> +#define REG_CMD0 0x00 /* Flash address */ >>>>>> +#define REG_CMD1 0x04 >>>>>> +#define REG_CMD2 0x08 >>>>>> +#define REG_CMD3 0x0c >>>>>> +#define REG_CTRL 0x10 /* Control Register */ >>>>>> +#define REG_ACT 0x14 /* AC Timing Register */ >>>>>> +#define REG_STR 0x18 /* Status Register */ >>>>>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>>>>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>>>>> +#define REG_RDST 0x28 /* Read Status Register */ >>>>>> +#define REG_REV 0x50 /* Revision Register */ >>>>>> +#define REG_FEA 0x54 /* Feature Register */ >>>>>> +#define REG_DATA 0x100 /* Data Register */ >>>>>> + >>>>> >>>>> These can just live in the C file - they are private to the implementation. >>> >>> No. Having them in a header means they can be reused by qtests. >> >> Do we really want to change the public/private boundaries of >> our source code just for the benefit of the test framework? Ugh. > > The truth is (and that applies to the hw/ refactoring series as well) > that we don't have a clear distinction between "public" and "private" > headers for devices. > I guess a logical conclusion here is that defines that relate to the programmers model (eg #define REG_ISR 0x0C/4) and parameterization (e.g. #define NUM_CHIP_SELECTS 4) need to be public while everything else (implementation stuff) is private. In theory however, the only client of a programmers model is unit testing. So perhaps there should be some sort of poisoning system like with the cpu headers? Your only allowed to include a header for a concrete class (such as this) if you own it, or if your a test. >>> To embed the device into a SoC object, the state struct and TYPE_ >>> constant should be in the header. That applies to each device part of >>> the SoC rather than on the board. >> I really think this is a step backwards. Every little lightweight device model needs its own header just for #define TYPE_FOO "foo" Regards, Peter >> I would like it if we could define what we consider to be best >> practice for public/private header files for QOM devices >> (in particular whether we need a public header for every device >> with the TYPE_ constants &c). > > gtk-doc for instance needs to scan one directory, so having all headers > in include/ would be beneficial long-term - today we do not get > generated documentation for qdev-core.h. > > The separation between PCI internal helpers and > structs/typedefs-to-be-used is made by including specific headers > depending on what you need. > > In the refcatoring thread I suggested that if we want to have all SPI > devices in hw/spi/ then each device that needs a public function > declaration or TYPE_ or struct should export its own header. If we group > them into a common directory such as hw/arm/ that could be shared across > SoC devices. Therefore I was suggesting not to split up too much but > seem to be overruled. ;) There's downsides to everything. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
2013/2/7 Peter Crosthwaite <peter.crosthwaite@xilinx.com>: > On Thu, Feb 7, 2013 at 4:59 PM, Kuo-Jung Su <dantesu@gmail.com> wrote: >> \ >> >> 2013/2/7 Peter Crosthwaite <peter.crosthwaite@xilinx.com>: >>> Hi Kuo-Jung, >>> >>> On Wed, Feb 6, 2013 at 7:45 PM, Kuo-Jung Su <dantesu@gmail.com> wrote: >>>> From: Kuo-Jung Su <dantesu@faraday-tech.com> >>>> >>>> The FTSPI020 is an integrated SPI Flash controller >>>> which supports upto 4 flash chips. >>>> >>>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com> >>>> --- >>>> hw/arm/Makefile.objs | 1 + >>>> hw/arm/faraday_a369.c | 12 ++ >>>> hw/arm/ftspi020.c | 345 +++++++++++++++++++++++++++++++++++++++++++++++++ >>>> hw/arm/ftspi020.h | 50 +++++++ >>>> 4 files changed, 408 insertions(+) >>>> create mode 100644 hw/arm/ftspi020.c >>>> create mode 100644 hw/arm/ftspi020.h >>>> >>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs >>>> index 508335a..7178b5d 100644 >>>> --- a/hw/arm/Makefile.objs >>>> +++ b/hw/arm/Makefile.objs >>>> @@ -52,3 +52,4 @@ obj-y += ftgmac100.o >>>> obj-y += ftlcdc200.o >>>> obj-y += fttsc010.o >>>> obj-y += ftsdc010.o >>>> +obj-y += ftspi020.o >>>> diff --git a/hw/arm/faraday_a369.c b/hw/arm/faraday_a369.c >>>> index 4ad48f5..132ee2f 100644 >>>> --- a/hw/arm/faraday_a369.c >>>> +++ b/hw/arm/faraday_a369.c >>>> @@ -203,6 +203,18 @@ a369_device_init(A369State *s) >>>> req = qdev_get_gpio_in(s->hdma[0], 13); >>>> qdev_connect_gpio_out(s->hdma[0], 13, ack); >>>> qdev_connect_gpio_out(ds, 0, req); >>>> + >>>> + /* ftspi020: as an external AHB device */ >>>> + ds = sysbus_create_simple("ftspi020", 0xC0000000, pic[13]); >>>> + spi = (SSIBus *)qdev_get_child_bus(ds, "spi"); >>>> + nr_flash = 1; >>>> + for (i = 0; i < nr_flash; i++) { >>>> + fl = ssi_create_slave_no_init(spi, "m25p80"); >>>> + qdev_prop_set_string(fl, "partname", "w25q64"); >>>> + qdev_init_nofail(fl); >>>> + cs_line = qdev_get_gpio_in(fl, 0); >>>> + sysbus_connect_irq(SYS_BUS_DEVICE(ds), i + 1, cs_line); >>>> + } >>>> } >>>> >>>> static void >>>> diff --git a/hw/arm/ftspi020.c b/hw/arm/ftspi020.c >>>> new file mode 100644 >>>> index 0000000..dab2f6f >>>> --- /dev/null >>>> +++ b/hw/arm/ftspi020.c >>>> @@ -0,0 +1,345 @@ >>>> +/* >>>> + * Faraday FTSPI020 Flash Controller >>>> + * >>>> + * Copyright (c) 2012 Faraday Technology >>>> + * Written by Dante Su <dantesu@faraday-tech.com> >>>> + * >>>> + * This code is licensed under GNU GPL v2+. >>>> + */ >>>> + >>>> +#include <hw/hw.h> >>>> +#include <sysemu/sysemu.h> >>>> +#include <hw/sysbus.h> >>>> +#include <hw/ssi.h> >>>> + >>>> +#include "ftspi020.h" >>>> + >>>> +#define TYPE_FTSPI020 "ftspi020" >>>> + >>>> +typedef struct Ftspi020State { >>>> + SysBusDevice busdev; >>>> + MemoryRegion iomem; >>>> + qemu_irq irq; >>>> + >>>> + /* DMA hardware handshake */ >>>> + qemu_irq req; >>>> + >>>> + SSIBus *spi; >>>> + uint8_t num_cs; >>> >>> Constant. No need for it to be part of the device state, unless you >>> want to drive it from a property and let the machine model decide how >>> my cs lines you have? >>> >> >> Got you! I'll move these stuff to the top of faraday_a36x.c as defines. >> >>>> + qemu_irq *cs_lines; >>>> + >>>> + int wip; /* SPI Flash Status: Write In Progress BIT shift */ >>>> + uint32_t datacnt; >>>> + >>>> + /* HW register caches */ >>>> + uint32_t cmd[4]; >>>> + uint32_t ctrl; >>>> + uint32_t timing; >>>> + uint32_t icr; >>>> + uint32_t isr; >>>> + uint32_t rdsr; >>>> +} Ftspi020State; >>>> + >>>> +#define FTSPI020(obj) \ >>>> + OBJECT_CHECK(Ftspi020State, obj, TYPE_FTSPI020) >>>> + >>>> +static void ftspi020_update_irq(Ftspi020State *s) >>>> +{ >>>> + if (s->isr) { >>>> + qemu_set_irq(s->irq, 1); >>>> + } else { >>>> + qemu_set_irq(s->irq, 0); >>>> + } >>>> +} >>>> + >>>> +static void ftspi020_handle_ack(void *opaque, int line, int level) >>>> +{ >>>> + Ftspi020State *s = FTSPI020(opaque); >>>> + >>>> + if (!(s->icr & 0x01)) { >>>> + return; >>>> + } >>>> + >>>> + if (level) { >>>> + qemu_set_irq(s->req, 0); >>>> + } else if (s->datacnt > 0) { >>>> + qemu_set_irq(s->req, 1); >>>> + } >>>> +} >>>> + >>>> +static int ftspi020_do_command(Ftspi020State *s) >>>> +{ >>>> + int cs = (s->cmd[3] >> 8) & 0x03; >>>> + int cmd = (s->cmd[3] >> 24) & 0xff; >>>> + int ilen = (s->cmd[1] >> 24) & 0x03; >>>> + int alen = (s->cmd[1] >> 0) & 0x07; >>>> + int dcyc = (s->cmd[1] >> 16) & 0xff; >>>> + >>> >>> bitops.h has helpers that can extract fields for you without all the >>>>> & stuff. Lots of magic numbers here as well, can you #define these >>> offsets with meaningful names? >> >> Got you! I'll add some BIT OFFSET and MACROs for these stuff. >> >>> >>>> + /* make sure the spi flash is de-activated */ >>>> + qemu_set_irq(s->cs_lines[cs], 1); >>>> + >>>> + /* activate the spi flash */ >>>> + qemu_set_irq(s->cs_lines[cs], 0); >>>> + >>>> + /* if it's a SPI flash READ_STATUS command */ >>>> + if ((s->cmd[3] & 0x06) == 0x04) { >>>> + do { >>>> + ssi_transfer(s->spi, cmd); >>>> + s->rdsr = ssi_transfer(s->spi, 0x00); >>>> + if (s->cmd[3] & 0x08) { >>>> + break; >>>> + } >>>> + } while (s->rdsr & (1 << s->wip)); >>>> + } else { >>>> + /* otherwise */ >>>> + int i; >>>> + >>>> + ilen = MIN(ilen, 2); >>>> + alen = MIN(alen, 4); >>>> + >>>> + /* command cycles */ >>>> + for (i = 0; i < ilen; ++i) { >>>> + ssi_transfer(s->spi, cmd); >>>> + } >>>> + /* address cycles */ >>>> + for (i = alen - 1; i >= 0; --i) { >>>> + ssi_transfer(s->spi, (s->cmd[0] >> (8 * i)) & 0xff); >>>> + } >>>> + /* dummy cycles */ >>>> + for (i = 0; i < (dcyc >> 3); ++i) { >>>> + ssi_transfer(s->spi, 0x00); >>>> + } >>>> + } >>>> + >>>> + if (s->datacnt <= 0) { >>> >>> == as this is unsigned. >> >> Got you! >> > > On second thoughts, just use > > if (!s->datacnt) { > > dont need ==. This is fairly standard for testing a down-to-0 counter. > got it, thanks >>> >>>> + qemu_set_irq(s->cs_lines[cs], 1); >>>> + } else if (s->icr & 0x01) { >>>> + qemu_set_irq(s->req, 1); >>>> + } >>>> + >>>> + if (s->cmd[3] & 0x01) { >>>> + s->isr |= 0x01; >>>> + } >>>> + ftspi020_update_irq(s); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void ftspi020_chip_reset(Ftspi020State *s) >>>> +{ >>>> + int i; >>>> + >>>> + s->datacnt = 0; >>>> + for (i = 0; i < 4; ++i) { >>>> + s->cmd[i] = 0; >>>> + } >>>> + s->wip = 0; >>>> + s->ctrl = 0; >>>> + s->timing = 0; >>>> + s->icr = 0; >>>> + s->isr = 0; >>>> + s->rdsr = 0; >>>> + >>>> + qemu_set_irq(s->irq, 0); >>> >>> Do the cs lines need resetting here? >>> >> >> Absolutely >> >>>> +} >>>> + >>>> +static uint64_t ftspi020_mem_read(void *opaque, hwaddr addr, unsigned size) >>>> +{ >>>> + Ftspi020State *s = FTSPI020(opaque); >>>> + uint64_t ret = 0; >>>> + >>>> + switch (addr) { >>>> + case REG_DATA: >>>> + if (!(s->cmd[3] & 0x02)) { >>>> + if (s->datacnt > 0) { >>>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 0; >>>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 8; >>>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 16; >>>> + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 24; >>> >>> loopable: >>> >>> for (i = 0; i < 4; ++i) { >>> ret = deposit32(ret, i * 8, 8, (uint32_t)(ssi_transfer(s->spi, >>> 0x00) & 0xff)); >>> } >>> >> >> Got you >> >>>> + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); >>>> + } >>>> + if (s->datacnt == 0) { >>>> + uint8_t cs = (s->cmd[3] >> 8) & 0x03; >>>> + qemu_set_irq(s->cs_lines[cs], 1); >>>> + if (s->cmd[3] & 0x01) { >>>> + s->isr |= 0x01; >>>> + } >>>> + ftspi020_update_irq(s); >>>> + } >>>> + } >>>> + break; >>>> + case REG_RDST: >>>> + return s->rdsr; >>>> + case REG_CMD0: >>> >>> the case .. syntax would allow you to collapse these 4 into 1: >>> >>> case REG_CMD0 .. REG_CMD3: >>> return s->cmd[(addr - REG_CMD0) / 4] >>> >> >> Got you >> >>>> + return s->cmd[0]; >>>> + case REG_CMD1: >>>> + return s->cmd[1]; >>>> + case REG_CMD2: >>>> + return s->cmd[2]; >>>> + case REG_CMD3: >>>> + return s->cmd[3]; >>>> + case REG_STR: >>>> + /* In QEMU, the data fifo is always ready for read/write */ >>>> + return 0x00000003; >>>> + case REG_ISR: >>>> + return s->isr; >>>> + case REG_ICR: >>>> + return s->icr; >>>> + case REG_CTRL: >>>> + return s->ctrl; >>>> + case REG_ACT: >>>> + return s->timing; >>>> + case REG_REV: >>>> + return 0x00010001; >>>> + case REG_FEA: >>>> + return 0x02022020; >>> >>> Few magic numbers here >>> >> >> case REG_REV: >> return 0x00010001; /* revision 1.0.1 */ > > ACK > >> case REG_FEA: >> return 0x02022020; /* Clock Mode=SYNC, cmd queue = 4, tx/rx fifo=32 */ >> > > You could these as three pieces as macros and | them together. I wont > block on it however. > >> However in QEMU, there is no I/O delay to access the underlying SPI flash, >> so the clock, and fifo depth information are totally useless. >> >>>> + default: >>>> + break; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static void ftspi020_mem_write(void *opaque, >>>> + hwaddr addr, >>>> + uint64_t val, >>>> + unsigned size) >>>> +{ >>>> + Ftspi020State *s = FTSPI020(opaque); >>>> + >>>> + switch (addr) { >>>> + case REG_DATA: >>>> + if (s->cmd[3] & 0x02) { >>>> + if (s->datacnt > 0) { >>>> + ssi_transfer(s->spi, (uint8_t)((val >> 0) & 0xff)); >>>> + ssi_transfer(s->spi, (uint8_t)((val >> 8) & 0xff)); >>>> + ssi_transfer(s->spi, (uint8_t)((val >> 16) & 0xff)); >>>> + ssi_transfer(s->spi, (uint8_t)((val >> 24) & 0xff)); >>>> + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); >>>> + } >>>> + if (s->datacnt == 0) { >>>> + uint8_t cs = (s->cmd[3] >> 8) & 0x03; >>>> + qemu_set_irq(s->cs_lines[cs], 1); >>>> + if (s->cmd[3] & 0x01) { >>>> + s->isr |= 0x01; >>>> + } >>>> + ftspi020_update_irq(s); >>>> + } >>>> + } >>>> + break; >>>> + case REG_CMD0: >>>> + s->cmd[0] = (uint32_t)val; >>>> + break; >>>> + case REG_CMD1: >>>> + s->cmd[1] = (uint32_t)val; >>>> + break; >>>> + case REG_CMD2: >>>> + s->datacnt = s->cmd[2] = (uint32_t)val; >>>> + break; >>>> + case REG_CMD3: >>>> + s->cmd[3] = (uint32_t)val; >>>> + ftspi020_do_command(s); >>>> + break; >>>> + case REG_ISR: >>>> + s->isr &= ~((uint32_t)val); >>>> + ftspi020_update_irq(s); >>>> + break; >>>> + case REG_ICR: >>>> + s->icr = (uint32_t)val; >>>> + break; >>>> + case REG_CTRL: >>>> + if (val & 0x100) { >>>> + ftspi020_chip_reset(s); >>>> + } >>>> + s->ctrl = (uint32_t)val & 0x70013; >>>> + s->wip = ((uint32_t)val >> 16) & 0x07; >>>> + break; >>>> + case REG_ACT: >>>> + s->timing = (uint32_t)val; >>>> + break; >>>> + default: >>>> + break; >>>> + } >>>> +} >>>> + >>>> +static const MemoryRegionOps ftspi020_ops = { >>>> + .read = ftspi020_mem_read, >>>> + .write = ftspi020_mem_write, >>>> + .endianness = DEVICE_LITTLE_ENDIAN, >>>> +}; >>>> + >>>> +static void ftspi020_reset(DeviceState *ds) >>>> +{ >>>> + SysBusDevice *busdev = SYS_BUS_DEVICE(ds); >>>> + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, busdev)); >>>> + >>>> + ftspi020_chip_reset(s); >>>> +} >>>> + >>>> +static int ftspi020_init(SysBusDevice *dev) >>>> +{ >>>> + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, dev)); >>>> + int i; >>>> + >>>> + memory_region_init_io(&s->iomem, >>>> + &ftspi020_ops, >>>> + s, >>>> + TYPE_FTSPI020, >>>> + 0x1000); >>>> + sysbus_init_mmio(dev, &s->iomem); >>>> + sysbus_init_irq(dev, &s->irq); >>>> + >>>> + s->spi = ssi_create_bus(&dev->qdev, "spi"); >>>> + s->num_cs = 4; >>>> + s->cs_lines = g_new(qemu_irq, s->num_cs); >>>> + ssi_auto_connect_slaves(DEVICE(s), s->cs_lines, s->spi); >>>> + for (i = 0; i < s->num_cs; ++i) { >>>> + sysbus_init_irq(dev, &s->cs_lines[i]); >>>> + } >>>> + >>>> + qdev_init_gpio_in(&s->busdev.qdev, ftspi020_handle_ack, 1); >>>> + qdev_init_gpio_out(&s->busdev.qdev, &s->req, 1); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const VMStateDescription vmstate_ftspi020 = { >>>> + .name = TYPE_FTSPI020, >>>> + .version_id = 1, >>>> + .minimum_version_id = 1, >>>> + .minimum_version_id_old = 1, >>>> + .fields = (VMStateField[]) { >>>> + VMSTATE_UINT32_ARRAY(cmd, Ftspi020State, 4), >>>> + VMSTATE_UINT32(ctrl, Ftspi020State), >>>> + VMSTATE_UINT32(timing, Ftspi020State), >>>> + VMSTATE_UINT32(icr, Ftspi020State), >>>> + VMSTATE_UINT32(isr, Ftspi020State), >>>> + VMSTATE_UINT32(rdsr, Ftspi020State), >>> >>> datacnt is missing. I think you need to save it as it is device state >>> that would need to be preserved if you machine got migrated in the >>> middle of a transaction. >>> >> >> Got you >> >>>> + VMSTATE_END_OF_LIST(), >>>> + } >>>> +}; >>>> + >>>> +static void ftspi020_class_init(ObjectClass *klass, void *data) >>>> +{ >>>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); >>>> + DeviceClass *dc = DEVICE_CLASS(klass); >>>> + >>>> + k->init = ftspi020_init; >>>> + dc->vmsd = &vmstate_ftspi020; >>>> + dc->reset = ftspi020_reset; >>>> + dc->no_user = 1; >>>> +} >>>> + >>>> +static const TypeInfo ftspi020_info = { >>>> + .name = TYPE_FTSPI020, >>>> + .parent = TYPE_SYS_BUS_DEVICE, >>>> + .instance_size = sizeof(Ftspi020State), >>>> + .class_init = ftspi020_class_init, >>>> +}; >>>> + >>>> +static void ftspi020_register_types(void) >>>> +{ >>>> + type_register_static(&ftspi020_info); >>>> +} >>>> + >>>> +type_init(ftspi020_register_types) >>>> diff --git a/hw/arm/ftspi020.h b/hw/arm/ftspi020.h >>>> new file mode 100644 >>>> index 0000000..a8a0930 >>>> --- /dev/null >>>> +++ b/hw/arm/ftspi020.h >>>> @@ -0,0 +1,50 @@ >>>> +/* >>>> + * Faraday FTSPI020 Flash Controller >>>> + * >>>> + * Copyright (c) 2012 Faraday Technology >>>> + * Written by Dante Su <dantesu@faraday-tech.com> >>>> + * >>>> + * This code is licensed under GNU GPL v2+. >>>> + */ >>>> + >>>> +#ifndef HW_ARM_FTSPI020_H >>>> +#define HW_ARM_FTSPI020_H >>>> + >>> >>> This device is not exporting any extensible APIs there is no need for >>> a header specific to this device. >>> >> >> Got you, but does this applied to ftrtc011 ? >> I remember someone had made a request to me to create header files >> for registers. Or maybe it's just a mis-understanding. >> > > Can you do me a fav and link me the discussion? > Please refer to the link bellow: http://patchwork.ozlabs.org/patch/213490/ It looks to me that's for RTC test, and so it might be RTC specific. And it's obviously that I forget to review this issue. >>>> +/****************************************************************************** >>>> + * FTSPI020 registers >>>> + *****************************************************************************/ >>>> +#define REG_CMD0 0x00 /* Flash address */ >>>> +#define REG_CMD1 0x04 >>>> +#define REG_CMD2 0x08 >>>> +#define REG_CMD3 0x0c >>>> +#define REG_CTRL 0x10 /* Control Register */ >>>> +#define REG_ACT 0x14 /* AC Timing Register */ >>>> +#define REG_STR 0x18 /* Status Register */ >>>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>>> +#define REG_RDST 0x28 /* Read Status Register */ >>>> +#define REG_REV 0x50 /* Revision Register */ >>>> +#define REG_FEA 0x54 /* Feature Register */ >>>> +#define REG_DATA 0x100 /* Data Register */ >>>> + >>> >>> These can just live in the C file - they are private to the implementation. >>> >> >> Same above >> >>>> +/****************************************************************************** >>>> + * Common SPI flash opcodes (Not FTSPI020 specific) >>>> + *****************************************************************************/ >>>> +#define OPCODE_WREN 0x06 /* Write enable */ >>>> +#define OPCODE_WRSR 0x01 /* Write status register 1 byte */ >>>> +#define OPCODE_RDSR 0x05 /* Read status register */ >>>> +#define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ >>>> +#define OPCODE_NORM_READ4 0x13 /* Read data bytes (4 bytes address) */ >>>> +#define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ >>>> +#define OPCODE_FAST_READ4 0x0c /* Read data bytes (4 bytes address) */ >>>> +#define OPCODE_PP 0x02 /* Page program (up to 256 bytes) */ >>>> +#define OPCODE_PP4 0x12 /* Page program (4 bytes address) */ >>>> +#define OPCODE_SE 0xd8 /* Sector erase (usually 64KiB) */ >>>> +#define OPCODE_SE4 0xdc /* Sector erase (4 bytes address) */ >>>> +#define OPCODE_RDID 0x9f /* Read JEDEC ID */ >>>> + >>> >>> This is repetition of M25P80s command opcodes. If anything, we should >>> factor these out into m25p80.h (doesnt exist yet) as this is the >>> second device model (apart from m25p80 itself) to need these. The >>> other one is xilinx_spips.c. However i notice almost all of these are >>> unused by your device model. Apart from the little bit of logic around >>> RDSR, is there any flash command-specific functionality in this >>> device? AFAICT it just accepts command code, address, num dummies and >>> payload size and the details of what those commands mean is abstracted >>> away from this hardware, making this table mostly redundant. If you >>> have future work that will need this that's another matter however. >>> >> >> You're absolutely right, the SPI flash commands has been abstracted. >> None of the OPCODE is used i the model. >> > > Then i think we delete the table in its entirety. The RDSR part can be > local to the C file (if needed see comment below). > >> About the RDSR, it requires the driver to set corresponding BIT in s->cmd[3] >> before issuing a RDSR(0x05) to underlying SPI flash device. >> > > Currently your hardware (as I read it) is snooping the requested > command to changing behavior based on if (cmd = RDSR). From what you > say here however, isnt this alternate behaviour triggered by a > s->cmd[3] bit field? I.e. can the correct bevhaviour of this device be > captured by your device watching s->cmd[3] rather than checking the > request command for the RDSR opcode? > > If this is the case then just delete the whole table and drive your > logic off your command registers. > Yes, the behaviour is triggered by a s->cmd[3] bit field, so the whole table will be deleted. >> These bits are >> >> BIT3 - 0: polling status by hardware, driver should polling the status >> register of FTSPI020 >> to check when the flash is back to idle state. (RDSR is >> sent only once) >> 1: polling status by hardware, driver would have to keep >> sending RDSR and >> read the status back to check if the flash is now idle. >> >> BIT2 - 0: this command is not a RDSR (READ_STATUS) >> 1: this command is a RDSR (READ_STATUS) >> >> BTW, tomorrow is the beginning of my Chinese New Year holidays, so >> please forgive me >> that I won't be able to make any response to the comments for 10 days. >> > > No rush. Enjoy your holidays. > > Regards, > Peter -- Best wishes, Kuo-Jung Su
2013/2/8 Peter Crosthwaite <peter.crosthwaite@xilinx.com>: > On Thu, Feb 7, 2013 at 10:13 PM, Andreas Färber <afaerber@suse.de> wrote: >> Am 07.02.2013 13:04, schrieb Peter Maydell: >>> On 7 February 2013 11:50, Andreas Färber <afaerber@suse.de> wrote: >>>> Am 07.02.2013 07:59, schrieb Kuo-Jung Su: >>>>>>> +/****************************************************************************** >>>>>>> + * FTSPI020 registers >>>>>>> + *****************************************************************************/ >>>>>>> +#define REG_CMD0 0x00 /* Flash address */ >>>>>>> +#define REG_CMD1 0x04 >>>>>>> +#define REG_CMD2 0x08 >>>>>>> +#define REG_CMD3 0x0c >>>>>>> +#define REG_CTRL 0x10 /* Control Register */ >>>>>>> +#define REG_ACT 0x14 /* AC Timing Register */ >>>>>>> +#define REG_STR 0x18 /* Status Register */ >>>>>>> +#define REG_ICR 0x20 /* Interrupt Control Register */ >>>>>>> +#define REG_ISR 0x24 /* Interrupt Status Register */ >>>>>>> +#define REG_RDST 0x28 /* Read Status Register */ >>>>>>> +#define REG_REV 0x50 /* Revision Register */ >>>>>>> +#define REG_FEA 0x54 /* Feature Register */ >>>>>>> +#define REG_DATA 0x100 /* Data Register */ >>>>>>> + >>>>>> >>>>>> These can just live in the C file - they are private to the implementation. >>>> >>>> No. Having them in a header means they can be reused by qtests. >>> >>> Do we really want to change the public/private boundaries of >>> our source code just for the benefit of the test framework? Ugh. >> >> The truth is (and that applies to the hw/ refactoring series as well) >> that we don't have a clear distinction between "public" and "private" >> headers for devices. >> > > I guess a logical conclusion here is that defines that relate to the > programmers model (eg #define REG_ISR 0x0C/4) and parameterization > (e.g. #define NUM_CHIP_SELECTS 4) need to be public while everything > else (implementation stuff) is private. In theory however, the only > client of a programmers model is unit testing. So perhaps there should > be some sort of poisoning system like with the cpu headers? Your only > allowed to include a header for a concrete class (such as this) if you > own it, or if your a test. > Got it, thanks. I'll try to update the header files and source files to meet the conclusion here. >>>> To embed the device into a SoC object, the state struct and TYPE_ >>>> constant should be in the header. That applies to each device part of >>>> the SoC rather than on the board. >>> > Did you mean to create a new DeviceState instance for SoC (e.g. faraday_a369_soc) and a new QEMUMachine instance for Target Board (e.g. faraday_a369_evb) If I'm correct, in this way, it would be easier to get the instance of parent object thru 'obj->parent' without breaking the QOM rules. The document I've found for QOM is a only abstract concept (http://wiki.qemu.org/Features/QOM) and since my english sucks, it's too difficult to me to understanding the concrete QOM implementation rules without example codes. BTW emulaing Faraday AHB remap function, it requires the items bellow to be accessible to these chips: SoC, DDR controller, AHB controller. 1. RAM size (Board parameter) 2. RAM base & window size (SoC parameter) 3. ROM base & window size (SoC parameter) 4. AHB address remap for ROM/RAM (SoC parameter) > I really think this is a step backwards. Every little lightweight > device model needs its own header just for > > #define TYPE_FOO "foo" > > Regards, > Peter > >>> I would like it if we could define what we consider to be best >>> practice for public/private header files for QOM devices >>> (in particular whether we need a public header for every device >>> with the TYPE_ constants &c). >> >> gtk-doc for instance needs to scan one directory, so having all headers >> in include/ would be beneficial long-term - today we do not get >> generated documentation for qdev-core.h. >> >> The separation between PCI internal helpers and >> structs/typedefs-to-be-used is made by including specific headers >> depending on what you need. >> >> In the refcatoring thread I suggested that if we want to have all SPI >> devices in hw/spi/ then each device that needs a public function >> declaration or TYPE_ or struct should export its own header. If we group >> them into a common directory such as hw/arm/ that could be shared across >> SoC devices. Therefore I was suggesting not to split up too much but >> seem to be overruled. ;) There's downsides to everything. >> >> Andreas >> >> -- >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >> -- Best wishes, Kuo-Jung Su
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs index 508335a..7178b5d 100644 --- a/hw/arm/Makefile.objs +++ b/hw/arm/Makefile.objs @@ -52,3 +52,4 @@ obj-y += ftgmac100.o obj-y += ftlcdc200.o obj-y += fttsc010.o obj-y += ftsdc010.o +obj-y += ftspi020.o diff --git a/hw/arm/faraday_a369.c b/hw/arm/faraday_a369.c index 4ad48f5..132ee2f 100644 --- a/hw/arm/faraday_a369.c +++ b/hw/arm/faraday_a369.c @@ -203,6 +203,18 @@ a369_device_init(A369State *s) req = qdev_get_gpio_in(s->hdma[0], 13); qdev_connect_gpio_out(s->hdma[0], 13, ack); qdev_connect_gpio_out(ds, 0, req); + + /* ftspi020: as an external AHB device */ + ds = sysbus_create_simple("ftspi020", 0xC0000000, pic[13]); + spi = (SSIBus *)qdev_get_child_bus(ds, "spi"); + nr_flash = 1; + for (i = 0; i < nr_flash; i++) { + fl = ssi_create_slave_no_init(spi, "m25p80"); + qdev_prop_set_string(fl, "partname", "w25q64"); + qdev_init_nofail(fl); + cs_line = qdev_get_gpio_in(fl, 0); + sysbus_connect_irq(SYS_BUS_DEVICE(ds), i + 1, cs_line); + } } static void diff --git a/hw/arm/ftspi020.c b/hw/arm/ftspi020.c new file mode 100644 index 0000000..dab2f6f --- /dev/null +++ b/hw/arm/ftspi020.c @@ -0,0 +1,345 @@ +/* + * Faraday FTSPI020 Flash Controller + * + * Copyright (c) 2012 Faraday Technology + * Written by Dante Su <dantesu@faraday-tech.com> + * + * This code is licensed under GNU GPL v2+. + */ + +#include <hw/hw.h> +#include <sysemu/sysemu.h> +#include <hw/sysbus.h> +#include <hw/ssi.h> + +#include "ftspi020.h" + +#define TYPE_FTSPI020 "ftspi020" + +typedef struct Ftspi020State { + SysBusDevice busdev; + MemoryRegion iomem; + qemu_irq irq; + + /* DMA hardware handshake */ + qemu_irq req; + + SSIBus *spi; + uint8_t num_cs; + qemu_irq *cs_lines; + + int wip; /* SPI Flash Status: Write In Progress BIT shift */ + uint32_t datacnt; + + /* HW register caches */ + uint32_t cmd[4]; + uint32_t ctrl; + uint32_t timing; + uint32_t icr; + uint32_t isr; + uint32_t rdsr; +} Ftspi020State; + +#define FTSPI020(obj) \ + OBJECT_CHECK(Ftspi020State, obj, TYPE_FTSPI020) + +static void ftspi020_update_irq(Ftspi020State *s) +{ + if (s->isr) { + qemu_set_irq(s->irq, 1); + } else { + qemu_set_irq(s->irq, 0); + } +} + +static void ftspi020_handle_ack(void *opaque, int line, int level) +{ + Ftspi020State *s = FTSPI020(opaque); + + if (!(s->icr & 0x01)) { + return; + } + + if (level) { + qemu_set_irq(s->req, 0); + } else if (s->datacnt > 0) { + qemu_set_irq(s->req, 1); + } +} + +static int ftspi020_do_command(Ftspi020State *s) +{ + int cs = (s->cmd[3] >> 8) & 0x03; + int cmd = (s->cmd[3] >> 24) & 0xff; + int ilen = (s->cmd[1] >> 24) & 0x03; + int alen = (s->cmd[1] >> 0) & 0x07; + int dcyc = (s->cmd[1] >> 16) & 0xff; + + /* make sure the spi flash is de-activated */ + qemu_set_irq(s->cs_lines[cs], 1); + + /* activate the spi flash */ + qemu_set_irq(s->cs_lines[cs], 0); + + /* if it's a SPI flash READ_STATUS command */ + if ((s->cmd[3] & 0x06) == 0x04) { + do { + ssi_transfer(s->spi, cmd); + s->rdsr = ssi_transfer(s->spi, 0x00); + if (s->cmd[3] & 0x08) { + break; + } + } while (s->rdsr & (1 << s->wip)); + } else { + /* otherwise */ + int i; + + ilen = MIN(ilen, 2); + alen = MIN(alen, 4); + + /* command cycles */ + for (i = 0; i < ilen; ++i) { + ssi_transfer(s->spi, cmd); + } + /* address cycles */ + for (i = alen - 1; i >= 0; --i) { + ssi_transfer(s->spi, (s->cmd[0] >> (8 * i)) & 0xff); + } + /* dummy cycles */ + for (i = 0; i < (dcyc >> 3); ++i) { + ssi_transfer(s->spi, 0x00); + } + } + + if (s->datacnt <= 0) { + qemu_set_irq(s->cs_lines[cs], 1); + } else if (s->icr & 0x01) { + qemu_set_irq(s->req, 1); + } + + if (s->cmd[3] & 0x01) { + s->isr |= 0x01; + } + ftspi020_update_irq(s); + + return 0; +} + +static void ftspi020_chip_reset(Ftspi020State *s) +{ + int i; + + s->datacnt = 0; + for (i = 0; i < 4; ++i) { + s->cmd[i] = 0; + } + s->wip = 0; + s->ctrl = 0; + s->timing = 0; + s->icr = 0; + s->isr = 0; + s->rdsr = 0; + + qemu_set_irq(s->irq, 0); +} + +static uint64_t ftspi020_mem_read(void *opaque, hwaddr addr, unsigned size) +{ + Ftspi020State *s = FTSPI020(opaque); + uint64_t ret = 0; + + switch (addr) { + case REG_DATA: + if (!(s->cmd[3] & 0x02)) { + if (s->datacnt > 0) { + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 0; + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 8; + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 16; + ret |= (uint32_t)(ssi_transfer(s->spi, 0x00) & 0xff) << 24; + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); + } + if (s->datacnt == 0) { + uint8_t cs = (s->cmd[3] >> 8) & 0x03; + qemu_set_irq(s->cs_lines[cs], 1); + if (s->cmd[3] & 0x01) { + s->isr |= 0x01; + } + ftspi020_update_irq(s); + } + } + break; + case REG_RDST: + return s->rdsr; + case REG_CMD0: + return s->cmd[0]; + case REG_CMD1: + return s->cmd[1]; + case REG_CMD2: + return s->cmd[2]; + case REG_CMD3: + return s->cmd[3]; + case REG_STR: + /* In QEMU, the data fifo is always ready for read/write */ + return 0x00000003; + case REG_ISR: + return s->isr; + case REG_ICR: + return s->icr; + case REG_CTRL: + return s->ctrl; + case REG_ACT: + return s->timing; + case REG_REV: + return 0x00010001; + case REG_FEA: + return 0x02022020; + default: + break; + } + + return ret; +} + +static void ftspi020_mem_write(void *opaque, + hwaddr addr, + uint64_t val, + unsigned size) +{ + Ftspi020State *s = FTSPI020(opaque); + + switch (addr) { + case REG_DATA: + if (s->cmd[3] & 0x02) { + if (s->datacnt > 0) { + ssi_transfer(s->spi, (uint8_t)((val >> 0) & 0xff)); + ssi_transfer(s->spi, (uint8_t)((val >> 8) & 0xff)); + ssi_transfer(s->spi, (uint8_t)((val >> 16) & 0xff)); + ssi_transfer(s->spi, (uint8_t)((val >> 24) & 0xff)); + s->datacnt = (s->datacnt < 4) ? 0 : (s->datacnt - 4); + } + if (s->datacnt == 0) { + uint8_t cs = (s->cmd[3] >> 8) & 0x03; + qemu_set_irq(s->cs_lines[cs], 1); + if (s->cmd[3] & 0x01) { + s->isr |= 0x01; + } + ftspi020_update_irq(s); + } + } + break; + case REG_CMD0: + s->cmd[0] = (uint32_t)val; + break; + case REG_CMD1: + s->cmd[1] = (uint32_t)val; + break; + case REG_CMD2: + s->datacnt = s->cmd[2] = (uint32_t)val; + break; + case REG_CMD3: + s->cmd[3] = (uint32_t)val; + ftspi020_do_command(s); + break; + case REG_ISR: + s->isr &= ~((uint32_t)val); + ftspi020_update_irq(s); + break; + case REG_ICR: + s->icr = (uint32_t)val; + break; + case REG_CTRL: + if (val & 0x100) { + ftspi020_chip_reset(s); + } + s->ctrl = (uint32_t)val & 0x70013; + s->wip = ((uint32_t)val >> 16) & 0x07; + break; + case REG_ACT: + s->timing = (uint32_t)val; + break; + default: + break; + } +} + +static const MemoryRegionOps ftspi020_ops = { + .read = ftspi020_mem_read, + .write = ftspi020_mem_write, + .endianness = DEVICE_LITTLE_ENDIAN, +}; + +static void ftspi020_reset(DeviceState *ds) +{ + SysBusDevice *busdev = SYS_BUS_DEVICE(ds); + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, busdev)); + + ftspi020_chip_reset(s); +} + +static int ftspi020_init(SysBusDevice *dev) +{ + Ftspi020State *s = FTSPI020(FROM_SYSBUS(Ftspi020State, dev)); + int i; + + memory_region_init_io(&s->iomem, + &ftspi020_ops, + s, + TYPE_FTSPI020, + 0x1000); + sysbus_init_mmio(dev, &s->iomem); + sysbus_init_irq(dev, &s->irq); + + s->spi = ssi_create_bus(&dev->qdev, "spi"); + s->num_cs = 4; + s->cs_lines = g_new(qemu_irq, s->num_cs); + ssi_auto_connect_slaves(DEVICE(s), s->cs_lines, s->spi); + for (i = 0; i < s->num_cs; ++i) { + sysbus_init_irq(dev, &s->cs_lines[i]); + } + + qdev_init_gpio_in(&s->busdev.qdev, ftspi020_handle_ack, 1); + qdev_init_gpio_out(&s->busdev.qdev, &s->req, 1); + + return 0; +} + +static const VMStateDescription vmstate_ftspi020 = { + .name = TYPE_FTSPI020, + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(cmd, Ftspi020State, 4), + VMSTATE_UINT32(ctrl, Ftspi020State), + VMSTATE_UINT32(timing, Ftspi020State), + VMSTATE_UINT32(icr, Ftspi020State), + VMSTATE_UINT32(isr, Ftspi020State), + VMSTATE_UINT32(rdsr, Ftspi020State), + VMSTATE_END_OF_LIST(), + } +}; + +static void ftspi020_class_init(ObjectClass *klass, void *data) +{ + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); + + k->init = ftspi020_init; + dc->vmsd = &vmstate_ftspi020; + dc->reset = ftspi020_reset; + dc->no_user = 1; +} + +static const TypeInfo ftspi020_info = { + .name = TYPE_FTSPI020, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(Ftspi020State), + .class_init = ftspi020_class_init, +}; + +static void ftspi020_register_types(void) +{ + type_register_static(&ftspi020_info); +} + +type_init(ftspi020_register_types) diff --git a/hw/arm/ftspi020.h b/hw/arm/ftspi020.h new file mode 100644 index 0000000..a8a0930 --- /dev/null +++ b/hw/arm/ftspi020.h @@ -0,0 +1,50 @@ +/* + * Faraday FTSPI020 Flash Controller + * + * Copyright (c) 2012 Faraday Technology + * Written by Dante Su <dantesu@faraday-tech.com> + * + * This code is licensed under GNU GPL v2+. + */ + +#ifndef HW_ARM_FTSPI020_H +#define HW_ARM_FTSPI020_H + +/****************************************************************************** + * FTSPI020 registers + *****************************************************************************/ +#define REG_CMD0 0x00 /* Flash address */ +#define REG_CMD1 0x04 +#define REG_CMD2 0x08 +#define REG_CMD3 0x0c +#define REG_CTRL 0x10 /* Control Register */ +#define REG_ACT 0x14 /* AC Timing Register */ +#define REG_STR 0x18 /* Status Register */ +#define REG_ICR 0x20 /* Interrupt Control Register */ +#define REG_ISR 0x24 /* Interrupt Status Register */ +#define REG_RDST 0x28 /* Read Status Register */ +#define REG_REV 0x50 /* Revision Register */ +#define REG_FEA 0x54 /* Feature Register */ +#define REG_DATA 0x100 /* Data Register */ + +/****************************************************************************** + * Common SPI flash opcodes (Not FTSPI020 specific) + *****************************************************************************/ +#define OPCODE_WREN 0x06 /* Write enable */ +#define OPCODE_WRSR 0x01 /* Write status register 1 byte */ +#define OPCODE_RDSR 0x05 /* Read status register */ +#define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ +#define OPCODE_NORM_READ4 0x13 /* Read data bytes (4 bytes address) */ +#define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ +#define OPCODE_FAST_READ4 0x0c /* Read data bytes (4 bytes address) */ +#define OPCODE_PP 0x02 /* Page program (up to 256 bytes) */ +#define OPCODE_PP4 0x12 /* Page program (4 bytes address) */ +#define OPCODE_SE 0xd8 /* Sector erase (usually 64KiB) */ +#define OPCODE_SE4 0xdc /* Sector erase (4 bytes address) */ +#define OPCODE_RDID 0x9f /* Read JEDEC ID */ + +/* Status Register bits. */ +#define SR_WIP 1 /* Write in progress */ +#define SR_WEL 2 /* Write enable latch */ + +#endif