qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Aurelien Jarno
Subject: Re: [Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching
Date: Wed, 27 Apr 2011 16:31:58 +0200
User-agent: Mutt/1.5.20 (2009-06-14)

On Tue, Apr 26, 2011 at 09:42:04AM +0200, Jan Kiszka wrote:
> 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 <address@hidden>
> >>> ---
> >>>  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 <address@hidden>
> > ---
> >  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.
> 

Done.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net



reply via email to

[Prev in Thread] Current Thread [Next in Thread]