[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/1] hw/block/m25p80: Fix Numonyx fast read dummy cycle co
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 1/1] hw/block/m25p80: Fix Numonyx fast read dummy cycle count |
Date: |
Thu, 29 Oct 2020 07:53:54 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 |
On 10/29/20 12:15 AM, Joe Komlodi wrote:
> Hi Philippe,
>
> Comments marked inline with [Joe]
>
> -----Original Message-----
> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On Behalf Of
> Philippe Mathieu-Daudé
> Sent: Wednesday, October 28, 2020 2:27 AM
> To: Joe Komlodi <komlodi@xilinx.com>; qemu-devel@nongnu.org
> Cc: Francisco Eduardo Iglesias <figlesia@xilinx.com>; kwolf@redhat.com;
> alistair@alistair23.me; qemu-block@nongnu.org; mreitz@redhat.com
> Subject: Re: [PATCH v2 1/1] hw/block/m25p80: Fix Numonyx fast read dummy
> cycle count
>
> Hi Joe,
>
> On 10/28/20 12:43 AM, Joe Komlodi wrote:
>> Numonyx chips determine the number of cycles to wait based on bits 7:4
>> in the volatile configuration register.
>>
>> However, if these bits are 0x0 or 0xF, the number of dummy cycles to
>> wait is
>> 10 on a QIOR or QIOR4 command, or 8 on any other currently supported
>> fast read command. [1]
>>
>> [1]
>> https://www.micron.com/-/media/client/global/documents/products/data-s
>> heet/ nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_02g_cbb_0.pdf
>> ?rev=9b167fbf2b3645efba6385949a72e453
>
> Please use single line for URL (even longer than 80 characters):
> https://www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_02g_cbb_0.pdf
>
> Or use
>
> Datasheet: "MT25QU02GCBB Rev. H 05/19 EN"
>
> [Joe] Ah, sorry about that, I'll put it all on one line in v3.
>
>> Page 34, page 39 note 5
>
> The note 5 is not restricted to QIOR/QIOR4 commands, so your patch seems
> incomplete.
>
> [Joe] Right. Right now it's only checking QIOR/QIOR4 because we don't have a
> way to put Numonyx chips in QIO mode (s->quad_enable == true), and we don't
> model DDR commands.
> Because of those restrictions, all the read commands need 8 cycles, except
> for QIOR/QIOR4.
>
>>
>> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
>> ---
>> hw/block/m25p80.c | 26 +++++++++++++++++++++++---
>> 1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
>> 483925f..302ed9d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -820,6 +820,26 @@ static void reset_memory(Flash *s)
>> trace_m25p80_reset_done(s);
>> }
>>
>> +static uint8_t numonyx_extract_cfg_num_dummies(Flash *s) {
>> + uint8_t cycle_count;
>> + uint8_t num_dummies;
>> + assert(get_man(s) == MAN_NUMONYX);
>> +
>> + cycle_count = extract32(s->volatile_cfg, 4, 4);
>
> Could be easier to review as:
>
> num_dummies = extract32(s->volatile_cfg, 4, 4);
>
> switch (s->cmd_in_progress) {
> /* note 5 comment ... */
> case FAST_READ:
> case ...
> /* field erased or reset in NVRAM */
> if (cycle_count == 0x0 || cycle_count == 0xf) {
> switch (s->cmd_in_progress) {
> case FAST_READ:
> case ...:
> num_dummies = 10;
> break;
> case ...:
> case ...:
> /* assert(s->quad_enable); */
> num_dummies = 8;
> break;
> default:
> qemu_log_mask(GUEST_ERROR, ...);
> break;
> }
> }
> break;
> default:
> break;
> }
>
> return num_dummies;
>
>> + if (cycle_count == 0x0 || cycle_count == 0x0F) {
>> + if (s->cmd_in_progress == QIOR || s->cmd_in_progress == QIOR4) {
>> + num_dummies = 10;
>> + } else {
>> + num_dummies = 8;
>
> Note, this is only true if s->quad_enable.
>
> Maybe clever to use the dumbest approach and copy the table...
>
> static uint8_t numonyx_extract_cfg_num_dummies(Flash *s) {
> static const unsigned dummy_clock_cycles[0x100][3] = {
> [FAST_READ] = {8, 8, 10},
> ...
> };
> uint8_t num_dummies = extract32(s->volatile_cfg, 4, 4);
>
> if (cycle_count == 0x0 || cycle_count == 0xf) {
> num_dummies = dummy_clock_cycles[s->cmd_in_progress][mode];
> }
>
> return num_dummies;
> }
>
> [Joe] I think either this or the switch statement would work fine, it just
> depends if we want to add a way to set s->quad_enable and s->dual_enable
> (doesn't exist in the model) for Numonyx chips.
> To be the most accurate, it probably would be best to add a way to
> enable/disable QIO and DIO mode for Numonyx chips, then change the cycles
> needed accordingly.
>
> Let me know what you think.
It is OK to not implement unused registers/fields, but please
use qemu_log_mask(LOG_UNIMP,...) there, so we can:
- notice the guest is accessing unimplemented registers at runtime
- easily find the missing place in the code.