[Top][All Lists]

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

Re: [Qemu-devel] [PATCH v3 3/3] Add partial flash interleaving to AMD CF

From: Michael Nawrocki
Subject: Re: [Qemu-devel] [PATCH v3 3/3] Add partial flash interleaving to AMD CFI devices
Date: Tue, 28 Nov 2017 14:47:44 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

On 11/23/2017 08:46 AM, Peter Maydell wrote:
On 13 November 2017 at 17:31, Mike Nawrocki
<address@hidden> wrote:
This mirrors the interleaving support already in place in
pflash_cfi01.c, using the max_device_width and device_width properties.

Signed-off-by: Mike Nawrocki <address@hidden>
  hw/block/pflash_cfi02.c | 244 +++++++++++++++++++++++++++++++++++++++++-------
  1 file changed, 209 insertions(+), 35 deletions(-)

Thanks for this patch. There is some code duplication from cfi01.c,
but I think it's ok not to worry about trying to share code.

      case 0xA0:
      case 0x10:
      case 0x30:
          /* Status register read */
          ret = pfl->status;
-        DPRINTF("%s: status %x\n", __func__, ret);
+        for (i = pfl->device_width;
+             i < pfl->bank_width; i += pfl->device_width) {
+            ret = deposit64(ret, 8 * i, 8 * pfl->device_width, ret);
+        }

This will loop forever in the legacy "device_width == 0" case,
won't it?

Ah, yeah looks like it. I'll add a check to fix this.

+        DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
          /* Toggle bit 6 */
          pfl->status ^= 0x40;
@@ -745,7 +901,25 @@ static Property pflash_cfi02_properties[] = {
      DEFINE_PROP_DRIVE("drive", struct pflash_t, blk),
      DEFINE_PROP_UINT32("num-blocks", struct pflash_t, nb_blocs, 0),
      DEFINE_PROP_UINT32("sector-length", struct pflash_t, sector_len, 0),
-    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
+    /* width here is the overall width of this QEMU device in bytes.
+     * The QEMU device may be emulating a number of flash devices
+     * wired up in parallel; the width of each individual flash
+     * device should be specified via device-width. If the individual
+     * devices have a maximum width which is greater than the width
+     * they are being used for, this maximum width should be set via
+     * max-device-width (which otherwise defaults to device-width).
+     * So for instance a 32-bit wide QEMU flash device made from four
+     * 16-bit flash devices used in 8-bit wide mode would be configured
+     * with width = 4, device-width = 1, max-device-width = 2.
+     *
+     * If device-width is not specified we default to backwards
+     * compatible behaviour which is a bad emulation of two
+     * 16 bit devices making up a 32 bit wide QEMU device. This
+     * is deprecated for new uses of this device.

Is this correct for pflash_cfi02 ? In cfi01 you can see that our
legacy behaviour does things like jamming two ID values
together into one response because it's doing this "bad emulation
of two 16 bit devices", but I don't see similar code in cfi02,
so maybe the legacy default is something different ?

In particular, if the legacy default is actually "just looks like
a single device" and the old code's behaviour is identical to
the new code with device_width 1, we may be able to simply set
the default width to 1 and avoid the nasty back-compat hacks.
(I had to put the hacks in for cfi01 because they really are
different behaviour, and we didn't want to potentially break
the use in the pc platform.)

-- PMM

I ran into this issue when emulating a PPMC7400 evaluation board. It has a 64-bit wide flash bus comprised of 4 16-bit AMD flash devices (in 16-bit mode), and the drivers I have send commands to all 4 devices simultaneously (by packing the data into quadwords). So it expects 4 responses packed into a quadword when it reads the response value.

As far as I can tell, the legacy default for pflash_cfi02 is "just looks like a single device". I think the legacy behavior is equivalent to setting device_width to bank_width in the new code; setting it to 1 in the new code would mean the response gets duplicated "bank_width" times while setting it to bank_width should result in a single response.

I was referencing pflash_cfi01, so I ended up duplicating the back-compat hacks, but as far as I can tell they are unnecessary here, and it'd be much cleaner to remove them.

I'll go ahead and remove the back-compat hacks and see if everything works as expected. I'll also incorporate your other suggestions and push a newer revision of the patch set as soon as possible.

reply via email to

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