Hello Marcin,

On 12/05/2016 04:33 PM, mar.krzeminski wrote:
> 
> 
> W dniu 05.12.2016 o 15:07, Cédric Le Goater pisze:
>> On 12/04/2016 05:31 PM, mar.krzeminski wrote:
>>> Hi Cedric,
>>>
>>> Since there is no public datasheet user guide for SMC I would ask some 
>>> question
>>> regarding HW itself because I got impression that you are implementing in 
>>> this
>>> model a part functionality that is done by Bootrom.
>>>
>>> W dniu 29.11.2016 o 16:43, Cédric Le Goater pisze:
>>>> The Aspeed SMC controllers have a mode (Command mode) in which
>>>> accesses to the flash content are no different than doing MMIOs. The
>>>> controller generates all the necessary commands to load (or store)
>>>> data in memory.
>>>>
>>>> However, accesses are restricted to the segment window assigned the
>>>> the flash module by the controller. This window is defined by the
>>>> Segment Address Register.
>>>>
>>>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>>>> Reviewed-by: Andrew Jeffery <and...@aj.id.au>
>>>> ---
>>>>  hw/ssi/aspeed_smc.c | 174 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>  1 file changed, 162 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>>>> index 72a44150b0a1..eec087199a22 100644
>>>> --- a/hw/ssi/aspeed_smc.c
>>>> +++ b/hw/ssi/aspeed_smc.c
>>>> @@ -69,6 +69,7 @@
>>>>  #define R_CTRL0           (0x10 / 4)
>>>>  #define   CTRL_CMD_SHIFT           16
>>>>  #define   CTRL_CMD_MASK            0xff
>>>> +#define   CTRL_AST2400_SPI_4BYTE   (1 << 13)
>>>>  #define   CTRL_CE_STOP_ACTIVE      (1 << 2)
>>>>  #define   CTRL_CMD_MODE_MASK       0x3
>>>>  #define     CTRL_READMODE          0x0
>>>> @@ -135,6 +136,16 @@
>>>>  #define ASPEED_SOC_SPI_FLASH_BASE   0x30000000
>>>>  #define ASPEED_SOC_SPI2_FLASH_BASE  0x38000000
>>>>  
>>>> +/* Flash opcodes. */
>>>> +#define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
>>>> +#define SPI_OP_WRDI       0x04    /* Write disable */
>>>> +#define SPI_OP_RDSR       0x05    /* Read status register */
>>>> +#define SPI_OP_WREN       0x06    /* Write enable */
>>> Are you sure that the controller is aware af all above commands (especially 
>>> WD/WE and RDS)?
>> HW is aware of SPI_OP_READ which is the default command for the 
>> "normal" read mode. For other modes, fast and write, the command 
>> op is configured in the CEx Control Register. 
>>
>> These ops are used in the model :
>>
>>  * SPI_OP_READ_FAST, for dummies
>>  * SPI_OP_EN4B, might be useless if we expect software to send
>>    this command before using this mode.
>>  * SPI_OP_WREN, same comment.
>>
>> The rest I should remove as it is unused.
> I think only SPI_OP_READ should stay in the model, rest goes to guest.

Well, we will need at least one 'EN4B' command to be sent for the qemu 
flash model to work. If the underlying m25p80 object does not know 
about the address width, the expected number of bytes will be wrong and
the result bogus. 

Same remark for the WREN, if the m25p80 object is not write enabled, 
modifying the flash content won't work.

Thanks,

C. 

>>
>>>> +
>>>> +/* Used for Macronix and Winbond flashes. */
>>>> +#define SPI_OP_EN4B       0xb7    /* Enter 4-byte mode */
>>>> +#define SPI_OP_EX4B       0xe9    /* Exit 4-byte mode */
>>>> +
>>> Same as above but I think 4byte address mode bit changes onlu nymber of 
>>> bytes
>>> that is sent after instruction.
>>>
>>>>  /*
>>>>   * Default segments mapping addresses and size for each slave per
>>>>   * controller. These can be changed when board is initialized with the
>>>> @@ -357,6 +368,98 @@ static inline bool aspeed_smc_is_writable(const 
>>>> AspeedSMCFlash *fl)
>>>>      return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + fl->id));
>>>>  }
>>>>  
>>>> +static inline int aspeed_smc_flash_cmd(const AspeedSMCFlash *fl)
>>>> +{
>>>> +    AspeedSMCState *s = fl->controller;
>>>> +    int cmd = (s->regs[s->r_ctrl0 + fl->id] >> CTRL_CMD_SHIFT) & 
>>>> CTRL_CMD_MASK;
>>>> +
>>>> +    /* This is the default value for read mode. In other modes, the
>>>> +     * command should be defined */
>>>> +    if (aspeed_smc_flash_mode(fl) == CTRL_READMODE) {
>>>> +        cmd = SPI_OP_READ;
>>>> +    }
>>>> +
>>>> +    if (!cmd) {
>>> cmd == 0 => NOP command for some flash (eg. mx66l1g45g).
>> So I should use another default value, OxFF ? 
> I believe this check is not needed at all because m25p80c will tell if it has
> unsupported instruction and HW should accept all register values, isn't it?
>>
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: no command defined for mode 
>>>> %d\n",
>>>> +                      __func__, aspeed_smc_flash_mode(fl));
>>>> +    }
>>>> +
>>>> +    return cmd;
>>>> +}
>>>> +
>>>> +static inline int aspeed_smc_flash_is_4byte(const AspeedSMCFlash *fl)
>>>> +{
>>>> +    AspeedSMCState *s = fl->controller;
>>>> +
>>>> +    if (s->ctrl->segments == aspeed_segments_spi) {
>>>> +        return s->regs[s->r_ctrl0] & CTRL_AST2400_SPI_4BYTE;
>>>> +    } else {
>>>> +        return s->regs[s->r_ce_ctrl] & (1 << (CTRL_EXTENDED0 + fl->id));
>>>> +    }
>>>> +}
>>>> +
>>>> +static void aspeed_smc_flash_select(const AspeedSMCFlash *fl)
>>>> +{
>>>> +    AspeedSMCState *s = fl->controller;
>>>> +
>>>> +    s->regs[s->r_ctrl0 + fl->id] &= ~CTRL_CE_STOP_ACTIVE;
>>>> +    qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
>>>> +}
>>>> +
>>>> +static void aspeed_smc_flash_unselect(const AspeedSMCFlash *fl)
>>>> +{
>>>> +    AspeedSMCState *s = fl->controller;
>>>> +
>>>> +    s->regs[s->r_ctrl0 + fl->id] |= CTRL_CE_STOP_ACTIVE;
>>>> +    qemu_set_irq(s->cs_lines[fl->id], aspeed_smc_is_ce_stop_active(fl));
>>>> +}
>>>> +
>>>> +static uint32_t aspeed_smc_check_segment_addr(AspeedSMCFlash *fl, 
>>>> uint32_t addr)
>>>> +{
>>>> +    AspeedSMCState *s = fl->controller;
>>>> +    AspeedSegments seg;
>>>> +
>>>> +    aspeed_smc_reg_to_segment(s->regs[R_SEG_ADDR0 + fl->id], &seg);
>>>> +    if ((addr & (seg.size - 1)) != addr) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "%s: invalid address 0x%08x for CS%d segment : "
>>>> +                      "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
>>>> +                      s->ctrl->name, addr, fl->id, seg.addr,
>>>> +                      seg.addr + seg.size);
>>>> +    }
>>>> +
>>>> +    addr &= seg.size - 1;
>>>> +    return addr;
>>>> +}
>>>> +
>>>> +static void aspeed_smc_flash_setup_read(AspeedSMCFlash *fl, uint32_t addr)
>>>> +{
>>>> +    AspeedSMCState *s = fl->controller;
>>>> +    uint8_t cmd = aspeed_smc_flash_cmd(fl);
>>>> +
>>>> +    /*
>>>> +     * To be checked: I am not sure the Aspeed SPI controller needs to
>>>> +     * enable writes when running in READ/FREAD command mode
>>>> +     */
>>>> +
>>>> +    /* access can not exceed CS segment */
>>>> +    addr = aspeed_smc_check_segment_addr(fl, addr);
>>>> +
>>>> +    /* TODO: do we have to send 4BYTE each time ? */
>>> I am not aware of any flash that needs that, this  command should be send 
>>> only once.
>> yes. That is what I think also.  
>>
>> it also means that a preliminary 4BYTE command should be 
>> sent before using that mode.
> Generally there are two ways to access more than 16MiB in flash:
> - 4byte address mode: all commands change to accept 4byte address, not 
> supported by all flash devices.
> - 4byte opcodes: different opcode is used to signal 4-byte long address (eg. 
> 0x3 three bytes, 0x13 four).
> Also not all flash support that. If the HW does not use any of those, ones 
> should be removed from this model.
> 
>>
>>> I also think that HW does not send this command (see above comment).
>>>
>>>> +    if (aspeed_smc_flash_is_4byte(fl)) {
>>>> +        ssi_transfer(s->spi, SPI_OP_EN4B);
>>>> +    }
>>>> +
>>>> +    ssi_transfer(s->spi, cmd);
>>>> +
>>>> +    if (aspeed_smc_flash_is_4byte(fl)) {
>>>> +        ssi_transfer(s->spi, (addr >> 24) & 0xff);
>>>> +    }
>>>> +    ssi_transfer(s->spi, (addr >> 16) & 0xff);
>>>> +    ssi_transfer(s->spi, (addr >> 8) & 0xff);
>>>> +    ssi_transfer(s->spi, (addr & 0xff));
>>>> +}
>>>> +
>>>>  static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned 
>>>> size)
>>>>  {
>>>>      AspeedSMCFlash *fl = opaque;
>>>> @@ -364,19 +467,55 @@ static uint64_t aspeed_smc_flash_read(void *opaque, 
>>>> hwaddr addr, unsigned size)
>>>>      uint64_t ret = 0;
>>>>      int i;
>>>>  
>>>> -    if (aspeed_smc_is_usermode(fl)) {
>>>> +    switch (aspeed_smc_flash_mode(fl)) {
>>>> +    case CTRL_USERMODE:
>>>>          for (i = 0; i < size; i++) {
>>>>              ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
>>>>          }
>>>> -    } else {
>>>> -        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
>>>> -                      __func__);
>>>> -        ret = -1;
>>>> +        break;
>>>> +    case CTRL_READMODE:
>>>> +    case CTRL_FREADMODE:
>>> CTRL_FREADMODE should not sent dummy bytes?
>> yes it should. this is in a following patch.
> Yes, I noticed that after sending this email :)
> 
> Thanks,
> Marcin
>>
>> Thanks,
>>
>> C. 
>>
>>> Thanks,
>>> Marcin
>>>> +        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);
>>>> +        }
>>>> +
>>>> +        aspeed_smc_flash_unselect(fl);
>>>> +        break;
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid flash mode %d\n",
>>>> +                      __func__, aspeed_smc_flash_mode(fl));
>>>>      }
>>>>  
>>>>      return ret;
>>>>  }
>>>>  
>>>> +static void aspeed_smc_flash_setup_write(AspeedSMCFlash *fl, uint32_t 
>>>> addr)
>>>> +{
>>>> +    AspeedSMCState *s = fl->controller;
>>>> +    uint8_t cmd = aspeed_smc_flash_cmd(fl);
>>>> +
>>>> +    /* Flash access can not exceed CS segment */
>>>> +    addr = aspeed_smc_check_segment_addr(fl, addr);
>>>> +
>>>> +    /* TODO: do we have to send 4BYTE each time ? */
>>>> +    if (aspeed_smc_flash_is_4byte(fl)) {
>>>> +        ssi_transfer(s->spi, SPI_OP_EN4B);
>>>> +    }
>>>> +
>>>> +    ssi_transfer(s->spi, SPI_OP_WREN);
>>>> +    ssi_transfer(s->spi, cmd);
>>>> +
>>>> +    if (aspeed_smc_flash_is_4byte(fl)) {
>>>> +        ssi_transfer(s->spi, (addr >> 24) & 0xff);
>>>> +    }
>>>> +    ssi_transfer(s->spi, (addr >> 16) & 0xff);
>>>> +    ssi_transfer(s->spi, (addr >> 8) & 0xff);
>>>> +    ssi_transfer(s->spi, (addr & 0xff));
>>>> +}
>>>> +
>>>>  static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t 
>>>> data,
>>>>                             unsigned size)
>>>>  {
>>>> @@ -390,14 +529,25 @@ static void aspeed_smc_flash_write(void *opaque, 
>>>> hwaddr addr, uint64_t data,
>>>>          return;
>>>>      }
>>>>  
>>>> -    if (!aspeed_smc_is_usermode(fl)) {
>>>> -        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
>>>> -                      __func__);
>>>> -        return;
>>>> -    }
>>>> +    switch (aspeed_smc_flash_mode(fl)) {
>>>> +    case CTRL_USERMODE:
>>>> +        for (i = 0; i < size; i++) {
>>>> +            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
>>>> +        }
>>>> +        break;
>>>> +    case CTRL_WRITEMODE:
>>>> +        aspeed_smc_flash_select(fl);
>>>> +        aspeed_smc_flash_setup_write(fl, addr);
>>>> +
>>>> +        for (i = 0; i < size; i++) {
>>>> +            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
>>>> +        }
>>>>  
>>>> -    for (i = 0; i < size; i++) {
>>>> -        ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
>>>> +        aspeed_smc_flash_unselect(fl);
>>>> +        break;
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid flash mode %d\n",
>>>> +                      __func__, aspeed_smc_flash_mode(fl));
>>>>      }
>>>>  }
>>>>  
>>
> 


Reply via email to