qemu-devel
[Top][All Lists]
Advanced

[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: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v3 3/3] Add partial flash interleaving to AMD CFI devices
Date: Thu, 23 Nov 2017 13:46:48 +0000

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?

> +        DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
>          /* Toggle bit 6 */
>          pfl->status ^= 0x40;
>          break;
> @@ -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.)

thanks
-- PMM



reply via email to

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