Re: [Qemu-devel] [PATCH V5 6/7] Fix CFI query responses for NOR flash

[Peter Maydell <peter.maydell@linaro.org>]

On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
> This change fixes the CFI query responses to handle NOR device
> widths that are different from the bank width.  Support is also
> added for multi-width devices in a x8 configuration.  This is
> typically x8/x16 devices, but the CFI specification mentions
> x8/x32 devices so those should be supported as well if they
> exist.
> The query response data is now replicated per-device in the bank,
> and is adjusted for x16 or x32 parts configured in x8 mode.
>
> The existing code is left in place for boards that have not
> been updated to specify an explicit device_width.  The VExpress
> board has been updated in an earlier patch in this series so
> this is the only board currently affected.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  hw/block/pflash_cfi01.c |  103 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 92 insertions(+), 11 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 8f81341..564e6ee 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -119,6 +119,66 @@ static void pflash_timer (void *opaque)
>      pfl->cmd = 0;
>  }
>
> +/* Perform a CFI query based on the bank width of the flash.
> + * If this code is called we know we have a device_width set for
> + * this flash.
> + */
> +static uint32_t pflash_cfi_query(pflash_t *pfl, hwaddr offset)
> +{
> +    int i;
> +    uint32_t resp = 0;
> +    hwaddr boff;
> +
> +    /* Adjust incoming offset to match expected device-width
> +     * addressing. CFI query addresses are always specified in terms of
> +     * the maximum supported width of the device.  This means that x8
> +     * devices and x8/x16 devices in x8 mode behave differently.  For
> +     * devices that are not used at their max width, we will be
> +     * provided with addresses that use higher address bits than
> +     * expected (based on the max width), so we will shift them lower
> +     * so that they will match the addresses used when
> +     * device_width==max_device_width.
> +     */
> +    boff = offset >> (ctz32(pfl->bank_width) +
> +                      ctz32(pfl->max_device_width) - ctz32(pfl->device_width));
> +
> +    if (boff > pfl->cfi_len) {
> +        return 0;
> +    }
> +    /* Now we will construct the CFI response generated by a single
> +     * device, then replicate that for all devices that make up the
> +     * bus.  For wide parts used in x8 mode, CFI query responses
> +     * are different than native byte-wide parts.
> +     */
> +    resp = pfl->cfi_table[boff];
> +    if (pfl->device_width != pfl->max_device_width) {
> +        /* The only case currently supported is x8 mode for a
> +         * wider part.
> +         */
> +        if (pfl->device_width != 1 || pfl->bank_width > 4) {
> +            DPRINTF("%s: Unsupported device configuration: device_width=%d, max_device_width=%d\n",

This line is overlong and needs a linebreak.


> +        boff = offset & 0xFF;
> +        if (pfl->bank_width == 2)
> +            boff = boff >> 1;
> +        else if (pfl->bank_width == 4)
> +            boff = boff >> 2;
> +

Missing braces.

Otherwise:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM