2013/2/7 Peter Crosthwaite <peter.crosthwa...@xilinx.com>: > 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. >
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 <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? > 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