hw/pflash_cfi02: Fix lazy reset of ROMD mode

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(-)

On Sun, Apr 03, 2011 at 01:16:26PM -0700, 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(-)

Thanks, applied.

> 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);
> -- 
> 1.7.1
> 
> 
>

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.

So the question is: Did you actually see a problem that the flash was
stuck in write mode, or did you just stumble over this strange code? In
the former case, please explain the sequence or provide a trace.

Thanks,
Jan

On Sun, Apr 10, 2011 at 01:38, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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).

Looking at the 2nd parameter to the call, and the
pflash_register_memory code, it seems it only makes sense with the
!pfl->rom_mode.

I thought that the goal was to allow multiple write operations before
exiting to rom mode, but that a read will return it to rom mode.  In
that case, this change seemed to fix it.

> We should
> instead switch back using the timer.
>
> So the question is: Did you actually see a problem that the flash was
> stuck in write mode, or did you just stumble over this strange code? In
> the former case, please explain the sequence or provide a trace.

I had enabled the debug trace messages and was probing the flash from
the efi shell on x86-64.  I found that I would always see the debug
messages for reads, after I started writing to the flash.

This change allowed the flash to go back to a rom mode where the debug
prints would then disappear for subsequent reads.

-Jordan