Re: [Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3715 bytes --]

On 2011-04-10 12:53, Jan Kiszka wrote:
> On 2011-04-10 10:38, Jan Kiszka wrote:
>> On 2011-04-03 22:16, Jordan Justen wrote:
>>> When checking pfl->rom_mode for when to lazily reenter ROMD mode,
>>> the value was check was the opposite of what it should have been.
>>> This prevent the part from returning to ROMD mode after a write
>>> was made to the CFI rom region.
>>>
>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>> ---
>>>  hw/pflash_cfi02.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>>> index 30c8aa4..370c5ee 100644
>>> --- a/hw/pflash_cfi02.c
>>> +++ b/hw/pflash_cfi02.c
>>> @@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>>>  
>>>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>>>      ret = -1;
>>> -    if (pfl->rom_mode) {
>>> +    if (!pfl->rom_mode) {
>>>          /* Lazy reset of to ROMD mode */
>>>          if (pfl->wcycle == 0)
>>>              pflash_register_memory(pfl, 1);
>>
>> Indeed, that block looks weird to its author today as well. But
>> inverting the logic completely defeats the purpose of lazy mode
>> switching (will likely file a patch to remove the block). We should
>> instead switch back using the timer.
> 
> That was wishful thinking. We were actually lacking a switch-back on
> read, something that the old twisted logic was papering over. Patch
> below should resolve that more gracefully, at least it does so here.
> 
> Jan
> 
> ------8<------
> 
> Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but
> resolved it by breaking that feature. This approach addresses the issue
> by switching back to ROMD after a certain amount of read accesses
> without further unlock sequences.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>  hw/pflash_cfi02.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 370c5ee..14bbc34 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -50,6 +50,8 @@ do {                                               \
>  #define DPRINTF(fmt, ...) do { } while (0)
>  #endif
>  
> +#define PFLASH_LAZY_ROMD_THRESHOLD 42
> +
>  struct pflash_t {
>      BlockDriverState *bs;
>      target_phys_addr_t base;
> @@ -70,6 +72,7 @@ struct pflash_t {
>      ram_addr_t off;
>      int fl_mem;
>      int rom_mode;
> +    int read_counter; /* used for lazy switch-back to rom mode */
>      void *storage;
>  };
>  
> @@ -112,10 +115,10 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
>  
>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>      ret = -1;
> -    if (!pfl->rom_mode) {
> -        /* Lazy reset of to ROMD mode */
> -        if (pfl->wcycle == 0)
> -            pflash_register_memory(pfl, 1);
> +    /* Lazy reset to ROMD mode after a certain amount of read accesses */
> +    if (!pfl->rom_mode && pfl->wcycle == 0 &&
> +        ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
> +        pflash_register_memory(pfl, 1);
>      }
>      offset &= pfl->chip_len - 1;
>      boff = offset & 0xFF;
> @@ -254,6 +257,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
>          /* Set the device in I/O access mode if required */
>          if (pfl->rom_mode)
>              pflash_register_memory(pfl, 0);
> +        pfl->read_counter = 0;
>          /* We're in read mode */
>      check_unlock0:
>          if (boff == 0x55 && cmd == 0x98) {

Please apply unless concerns remain.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]