Re: [Qemu-devel] [PATCH for-2.9 22/30] aspeed/smc: handle dummy bytes when doing fast reads

["Cédric Le Goater" <clg@kaod.org>]

On 12/04/2016 05:46 PM, mar.krzeminski wrote:
> 
> 
> W dniu 29.11.2016 o 16:44, Cédric Le Goater pisze:
>> When doing fast read, a certain amount of dummy bytes should be sent
>> before the read. This number is configurable in the controler CE0
>> Control Register and needs to be modeled using fake transfers the
>> flash module.
>>
>> When the controller is configured for Command mode, the SPI command
>> used to do the read is stored in the CE0 control register but, in User
>> mode, we need to snoop into the flow of bytes to catch the command. It
>> should be the first byte after CS select.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>>  hw/ssi/aspeed_smc.c         | 58 ++++++++++++++++++++++++++++++++++++++-------
>>  include/hw/ssi/aspeed_smc.h |  1 +
>>  2 files changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 9596ea94a3bc..733fd8b09c06 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -71,7 +71,9 @@
>>  #define R_CTRL0           (0x10 / 4)
>>  #define   CTRL_CMD_SHIFT           16
>>  #define   CTRL_CMD_MASK            0xff
>> +#define   CTRL_DUMMY_HIGH_SHIFT    14
>>  #define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
>> +#define   CTRL_DUMMY_LOW_SHIFT     6 /* 2 bits [7:6] */
>>  #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
>>  #define   CTRL_CMD_MODE_MASK       0x3
>>  #define     CTRL_READMODE          0x0
>> @@ -151,6 +153,7 @@
>>  #define SPI_OP_WRDI       0x04    /* Write disable */
>>  #define SPI_OP_RDSR       0x05    /* Read status register */
>>  #define SPI_OP_WREN       0x06    /* Write enable */
>> +#define SPI_OP_READ_FAST  0x0b    /* Read data bytes (high frequency) */
>>  
>>  /* Used for Macronix and Winbond flashes. */
>>  #define SPI_OP_EN4B       0xb7    /* Enter 4-byte mode */
>> @@ -510,6 +513,12 @@ static void aspeed_smc_flash_setup_read(AspeedSMCFlash *fl, uint32_t addr)
>>      /* access can not exceed CS segment */
>>      addr = aspeed_smc_check_segment_addr(fl, addr);
>>  
>> +    /*
>> +     * Remember command as we might need to send dummy bytes before
>> +     * reading data
>> +     */
>> +    fl->cmd = cmd;
>> +
>>      /* TODO: do we have to send 4BYTE each time ? */
>>      if (aspeed_smc_flash_is_4byte(fl)) {
>>          ssi_transfer(s->spi, SPI_OP_EN4B);
>> @@ -525,27 +534,50 @@ static void aspeed_smc_flash_setup_read(AspeedSMCFlash *fl, uint32_t addr)
>>      ssi_transfer(s->spi, (addr & 0xff));
>>  }
>>  
>> +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
>> +{
>> +    AspeedSMCState *s = fl->controller;
>> +    uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
>> +    uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
>> +    uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
>> +
>> +    return ((dummy_high << 2) | dummy_low) * 8;
>> +}
>> +
>> +static uint64_t aspeed_smc_flash_do_read(AspeedSMCFlash *fl, unsigned size)
>> +{
>> +    AspeedSMCState *s = fl->controller;
>> +    uint64_t ret = 0;
>> +    int i;
>> +
>> +    if (fl->cmd == SPI_OP_READ_FAST) {
>> +        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
>> +            ssi_transfer(s->spi, 0x0);
>> +        }
>> +    }
> Generally this is wrong, controller should be not aware of any command in user mode.
> Currently you are forced to do this by m25p80c dummy cycles implementation.
> I had no time to improve this since it need to update in Xilinx models, but maybe it is good place to talk about the solution.
> I was thinking to add to ssi function transferN, and use it in flash models.
> Firs introduce new state for dummy cycles in m25p80 and then:

This new state would depend on the command op ? fastread would put
the object in a dummy_cycle state ? 

> a) in case caller use ssi_transfer(transfer8 in flsh models) dummy count will be incremented by 8.
> This should solve the problem when flash is connected to controller that is not aware about dummy cycles,
> like this mode or clear SPI controller.
> b) when caller use ssi_trasferN length (in bits) will be the number of dummy cycles.
> 
> What is your opinion?

when you have some time, please send a quick rfc patch for the API :) 
so that I can experiment on the aspeed controller.

Thanks,

C. 

>> +    fl->cmd = 0;
>> +
>> +    for (i = 0; i < size; i++) {
>> +        ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
>> +    }
>> +    return ret;
>> +}
>> +
>>  static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>>  {
>>      AspeedSMCFlash *fl = opaque;
>> -    const AspeedSMCState *s = fl->controller;
>>      uint64_t ret = 0;
>> -    int i;
>>  
>>      switch (aspeed_smc_flash_mode(fl)) {
>>      case CTRL_USERMODE:
>> -        for (i = 0; i < size; i++) {
>> -            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
>> -        }
>> +        ret = aspeed_smc_flash_do_read(fl, size);
>>          break;
>>      case CTRL_READMODE:
>>      case CTRL_FREADMODE:
>>          aspeed_smc_flash_select(fl);
>>          aspeed_smc_flash_setup_read(fl, addr);
>>  
>> -        for (i = 0; i < size; i++) {
>> -            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
>> -        }
>> +        ret = aspeed_smc_flash_do_read(fl, size);
>>  
>>          aspeed_smc_flash_unselect(fl);
>>          break;
>> @@ -596,6 +628,15 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
>>  
>>      switch (aspeed_smc_flash_mode(fl)) {
>>      case CTRL_USERMODE:
>> +        /*
>> +         * First write after chip select is the chip command. Remember
>> +         * it as we might need to send dummy bytes before reading
>> +         * data. It will be reseted when the chip is unselected.
>> +         */
>> +        if (!fl->cmd) {
>> +            fl->cmd = data & 0xff;
>> +        }
>> +
>>          for (i = 0; i < size; i++) {
>>              ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
>>          }
>> @@ -629,6 +670,7 @@ static const MemoryRegionOps aspeed_smc_flash_ops = {
>>  static void aspeed_smc_flash_update_cs(AspeedSMCFlash *fl)
>>  {
>>      AspeedSMCState *s = fl->controller;
>> +    fl->cmd = 0;
>>      qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
>>  }
>>  
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index 88a904849801..3ae0a369073d 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -52,6 +52,7 @@ typedef struct AspeedSMCFlash {
>>  
>>      uint8_t id;
>>      uint32_t size;
>> +    uint8_t cmd;
>>  
>>      MemoryRegion mmio;
>>      DeviceState *flash;
>