From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48321) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEcuc-00014t-If for qemu-devel@nongnu.org; Tue, 26 Apr 2011 03:42:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QEcuW-0002mN-Og for qemu-devel@nongnu.org; Tue, 26 Apr 2011 03:42:22 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:34905) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEcuW-0002mA-Ca for qemu-devel@nongnu.org; Tue, 26 Apr 2011 03:42:16 -0400 Message-ID: <4DB6774C.7010608@web.de> Date: Tue, 26 Apr 2011 09:42:04 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1301861786-6637-1-git-send-email-jordan.l.justen@intel.com> <4DA16C6A.7090303@web.de> <4DA18C33.6050002@web.de> In-Reply-To: <4DA18C33.6050002@web.de> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig802523960757E27F092B05E0" Sender: jan.kiszka@web.de Subject: Re: [Qemu-devel] [PATCH] pflash: Restore & fix lazy ROMD switching List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Aurelien Jarno Cc: Jordan Justen This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig802523960757E27F092B05E0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable 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 >>> --- >>> 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, targe= t_phys_addr_t offset, >>> =20 >>> DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset); >>> ret =3D -1; >>> - if (pfl->rom_mode) { >>> + if (!pfl->rom_mode) { >>> /* Lazy reset of to ROMD mode */ >>> if (pfl->wcycle =3D=3D 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. >=20 > 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. >=20 > Jan >=20 > ------8<------ >=20 > Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, bu= t > 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. >=20 > Signed-off-by: Jan Kiszka > --- > hw/pflash_cfi02.c | 12 ++++++++---- > 1 files changed, 8 insertions(+), 4 deletions(-) >=20 > 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 > =20 > +#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; > }; > =20 > @@ -112,10 +115,10 @@ static uint32_t pflash_read (pflash_t *pfl, targe= t_phys_addr_t offset, > =20 > DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset); > ret =3D -1; > - if (!pfl->rom_mode) { > - /* Lazy reset of to ROMD mode */ > - if (pfl->wcycle =3D=3D 0) > - pflash_register_memory(pfl, 1); > + /* Lazy reset to ROMD mode after a certain amount of read accesses= */ > + if (!pfl->rom_mode && pfl->wcycle =3D=3D 0 && > + ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) { > + pflash_register_memory(pfl, 1); > } > offset &=3D pfl->chip_len - 1; > boff =3D offset & 0xFF; > @@ -254,6 +257,7 @@ static void pflash_write (pflash_t *pfl, target_phy= s_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 =3D 0; > /* We're in read mode */ > check_unlock0: > if (boff =3D=3D 0x55 && cmd =3D=3D 0x98) { Please apply unless concerns remain. Jan --------------enig802523960757E27F092B05E0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.15 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/ iEYEARECAAYFAk22d0wACgkQitSsb3rl5xTEUgCg4QeYoaEK141sEVLJuYuVeD98 RMwAnR+5ecB/npYt1tqdJiESUOpjIC8V =Q5dI -----END PGP SIGNATURE----- --------------enig802523960757E27F092B05E0--