Re: [Qemu-devel] [PATCH for-2.9 17/30] aspeed/smc: handle SPI flash Command mode

["mar.krzeminski" <mar.krzeminski@gmail.com>]

Hello Cedric,

W dniu 02.01.2017 o 16:56, Cédric Le Goater pisze:
> 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 <clg@kaod.org>
>>>>> Reviewed-by: Andrew Jeffery <andrew@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.
Hmm, most of the flash I know by default use 3byte address mode.
What flash are you connecting to model. Do you have same on HW?
>
> Same remark for the WREN, if the m25p80 object is not write enabled,
> modifying the flash content won't work.
Same as above.

Thanks,
Marcin
>
> 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));
>>>>>       }
>>>>>   }
>>>>>   
>