Re: [PATCH 7/9] hw/block/pflash_cfi02: Factor out DeviceReset method

[Philippe Mathieu-Daudé]

On 3/10/21 9:48 AM, Bin Meng wrote:
> On Wed, Mar 10, 2021 at 7:55 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> There is multiple places doing a device reset. Factor that
>> out in a common method which matches the DeviceReset prototype,
>> so we can also remove the reset code from the DeviceRealize()
>> handler. Explicit the device is set in "read array" mode on
>> reset.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/pflash_cfi02.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index 2ba77a0171b..484b056b898 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -193,6 +193,14 @@ static void pflash_mode_read_array(PFlashCFI02 *pfl)
>>      memory_region_rom_device_set_romd(&pfl->orig_mem, true);
>>  }
>>
>> +static void pflash_cfi02_reset(DeviceState *dev)
>> +{
>> +    PFlashCFI02 *pfl = PFLASH_CFI02(dev);
>> +
>> +    trace_pflash_reset();
>> +    pflash_mode_read_array(pfl);
>> +}
>> +
>>  static size_t pflash_regions_count(PFlashCFI02 *pfl)
>>  {
>>      return pfl->cfi_table[0x2c];
>> @@ -330,8 +338,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
>> unsigned int width)
>>      default:
>>          /* This should never happen : reset state & treat it as a read*/
>>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>> -        pfl->wcycle = 0;
>> -        pfl->cmd = 0;
>> +        pflash_cfi02_reset(DEVICE(pfl));
>>          /* fall through to the read code */
>>      case 0x80: /* Erase (unlock) */
>>          /* We accept reads during second unlock sequence... */
>> @@ -710,10 +717,8 @@ static void pflash_write(void *opaque, hwaddr offset, 
>> uint64_t value,
>>
>>      /* Reset flash */
>>   reset_flash:
>> -    trace_pflash_reset();
>>      pfl->bypass = 0;
>> -    pfl->wcycle = 0;
>> -    pfl->cmd = 0;
>> +    pflash_cfi02_reset(DEVICE(pfl));
> 
> The old codes did not set pfl->rom_mode to true, but the new codes
> pflash_cfi02_reset() do. Is this correct?

Hmmm let's be precautious indeed. I'll better split this change.