On Thu, Feb 7, 2013 at 4:59 PM, Kuo-Jung Su <dant...@gmail.com> wrote: > \ > > 2013/2/7 Peter Crosthwaite <peter.crosthwa...@xilinx.com>: >> Hi Kuo-Jung, >> >> On Wed, Feb 6, 2013 at 7:45 PM, Kuo-Jung Su <dant...@gmail.com> wrote: >>> From: Kuo-Jung Su <dant...@faraday-tech.com> >>> >>> The FTSPI020 is an integrated SPI Flash controller >>> which supports upto 4 flash chips. >>> >>> Signed-off-by: Kuo-Jung Su <dant...@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 <dant...@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 <dant...@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