[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] m25p80: Add basic support for the SFDP command
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH 1/2] m25p80: Add basic support for the SFDP command |
Date: |
Tue, 1 Sep 2020 17:22:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
On 9/1/20 3:36 PM, Francisco Iglesias wrote:
> Hi Cedric,
>
> On [2020 Aug 27] Thu 11:11:29, Cédric Le Goater wrote:
>> JEDEC STANDARD JESD216 for Serial Flash Discovery Parameters (SFDP)
>> provides a mean to describe the features of a serial flash device
>> using a set of internal parameter tables.
>>
>> This is the initial framework for the RDSFDP command giving access to
>> a private SFDP area under the flash. This area now needs to be
>> populated with the flash device characteristics, using a new 'sfdp'
>> pointer under FlashPartInfo.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/block/m25p80_sfdp.h | 15 +++++++++++++++
>> hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++++++
>> hw/block/trace-events | 1 +
>> 3 files changed, 49 insertions(+)
>> create mode 100644 hw/block/m25p80_sfdp.h
>>
>> diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
>> new file mode 100644
>> index 000000000000..b75fd0b0c13f
>> --- /dev/null
>> +++ b/hw/block/m25p80_sfdp.h
>> @@ -0,0 +1,15 @@
>> +/*
>> + * M25P80 SFDP
>> + *
>> + * Copyright (c) 2020, IBM Corporation.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef HW_M25P80_SFDP_H
>> +#define HW_M25P80_SFDP_H
>> +
>> +#define M25P80_SFDP_AREA_SIZE 0x100
>> +
>> +#endif
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 82270884416e..32925589ec7a 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -33,6 +33,7 @@
>> #include "qemu/error-report.h"
>> #include "qapi/error.h"
>> #include "trace.h"
>> +#include "m25p80_sfdp.h"
>>
>> /* Fields for FlashPartInfo->flags */
>>
>> @@ -72,6 +73,7 @@ typedef struct FlashPartInfo {
>> * This field inform how many die is in the chip.
>> */
>> uint8_t die_cnt;
>> + const uint8_t *sfdp;
>> } FlashPartInfo;
>>
>> /* adapted from linux */
>> @@ -333,6 +335,7 @@ typedef enum {
>> BULK_ERASE = 0xc7,
>> READ_FSR = 0x70,
>> RDCR = 0x15,
>> + RDSFDP = 0x5a,
>>
>> READ = 0x03,
>> READ4 = 0x13,
>> @@ -398,6 +401,7 @@ typedef enum {
>> STATE_COLLECTING_DATA,
>> STATE_COLLECTING_VAR_LEN_DATA,
>> STATE_READING_DATA,
>> + STATE_READING_SFDP,
>> } CMDState;
>>
>> typedef enum {
>> @@ -619,6 +623,8 @@ static inline int get_addr_length(Flash *s)
>> }
>>
>> switch (s->cmd_in_progress) {
>> + case RDSFDP:
>> + return 3;
>> case PP4:
>> case PP4_4:
>> case QPP_4:
>> @@ -744,6 +750,17 @@ static void complete_collecting_data(Flash *s)
>> " by device\n");
>> }
>> break;
>> +
>> + case RDSFDP:
>> + if (s->cur_addr < M25P80_SFDP_AREA_SIZE) {
>
> Is perhaps M25P80_SFDP_AREA_SIZE a limit for the micron flashes in patch
> 2 (and not sfdp)?
No. It's a common limit of the chips available on the systems I have
access to. Nothing is said in the specs on a maximum size and only
one table is required. But, theoretically, we have a 24bits address
space.
> An option might be to change .sfdp to a '.sfdp_read' function decoding
> the address internally (for the micron flashes it could just return the
> array value in case the address is less than M25P80_SFDP_AREA_SIZE
Yes. This is one way to make the SFDP table size depend on the chip
model.
> or 0xFF else).
Some flash seems to wrap around when the max size is reached. See
the mx25l25635e in my git tree. But I have only read 0x100 bytes.
> It might become easier
> to add flashes containing parameter tables in higher addresses (the
> function could then decode the address into a specific table/area).
yes. Something like that.
>> + s->state = STATE_READING_SFDP;
>> + } else {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "M25P80: Invalid SFDP address %#" PRIx32 "\n",
>> + s->cur_addr);
>> + }
>> + break;
>> +
>> default:
>> break;
>> }
>> @@ -1160,6 +1177,16 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>> case RSTQIO:
>> s->quad_enable = false;
>> break;
>> + case RDSFDP:
>> + if (s->pi->sfdp) {
>> + s->needed_bytes = get_addr_length(s) + 1 ; /* SFDP addr + dummy
>> */
>
> Should above be changed to:
>
> s->needed_bytes = get_addr_length(s) + 8; /* SFDP addr + dummy */
oops. I have been lucky. I need to change the Aspeed SMC snooping
routine to compute the number of dummies for the RDSFDP command.
> (I think it might fail else when the flashes are operating in 2 lines and
> 4 lines and are generating 8 dummy cycles for the RDSFDP)
The specs say that the requirement are independent of the I/O mode.
4.5 Instruction Input Modes
The Read SFDP instruction can be used with device supported modes
of (1-1-1), (2-2-2), or (4-4-4), but the opcode (0x5A), address
(24 bits), eight dummy clocks (8 wait states), and 50 MHz
requirements remain the same.
Thanks,
C.
> Best regards,
> Francisco Iglesias
>
>> + s->pos = 0;
>> + s->len = 0;
>> + s->state = STATE_COLLECTING_DATA;
>> + break;
>> + }
>> + /* Fallthrough */
>> +
>> default:
>> s->pos = 0;
>> s->len = 1;
>> @@ -1256,6 +1283,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss,
>> uint32_t tx)
>> }
>> }
>> break;
>> + case STATE_READING_SFDP:
>> + assert(s->pi->sfdp);
>> + r = s->pi->sfdp[s->cur_addr];
>> + trace_m25p80_read_sfdp(s, s->cur_addr, (uint8_t)r);
>> + s->cur_addr = (s->cur_addr + 1) & (M25P80_SFDP_AREA_SIZE - 1);
>> + break;
>>
>> default:
>> case STATE_IDLE:
>> diff --git a/hw/block/trace-events b/hw/block/trace-events
>> index 958fcc5508d1..53d377ca2b46 100644
>> --- a/hw/block/trace-events
>> +++ b/hw/block/trace-events
>> @@ -152,5 +152,6 @@ m25p80_page_program(void *s, uint32_t addr, uint8_t tx)
>> "[%p] page program cur_a
>> m25p80_transfer(void *s, uint8_t state, uint32_t len, uint8_t needed,
>> uint32_t pos, uint32_t cur_addr, uint8_t t) "[%p] Transfer state 0x%"PRIx8"
>> len 0x%"PRIx32" needed 0x%"PRIx8" pos 0x%"PRIx32" addr 0x%"PRIx32" tx
>> 0x%"PRIx8
>> m25p80_read_byte(void *s, uint32_t addr, uint8_t v) "[%p] Read byte
>> 0x%"PRIx32"=0x%"PRIx8
>> m25p80_read_data(void *s, uint32_t pos, uint8_t v) "[%p] Read data
>> 0x%"PRIx32"=0x%"PRIx8
>> +m25p80_read_sfdp(void *s, uint32_t addr, uint8_t v) "[%p] Read SFDP
>> 0x%"PRIx32"=0x%"PRIx8
>> m25p80_binding(void *s) "[%p] Binding to IF_MTD drive"
>> m25p80_binding_no_bdrv(void *s) "[%p] No BDRV - binding to RAM"
>> --
>> 2.25.4
>>