Re: [Qemu-devel] [PATCH v3 20/20] arm: add Faraday FTSPI020 SPI flash controller support

[Peter Crosthwaite]

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