Re: [RFC PATCH v2 2/2] hw/arm/integratorcp: Map a CFI parallel flash

[Philippe Mathieu-Daudé]

On 2/25/20 1:47 PM, Peter Maydell wrote:
> On Sun, 23 Feb 2020 at 23:30, Philippe Mathieu-Daudé <address@hidden> wrote:
> > The Linux kernel displays errors why trying to detect the flash:
> >
> >    Linux version 4.16.0 (linus@genomnajs) (gcc version 7.2.1 20171011 (Linaro 
> > GCC 7.2-2017.11)) #142 PREEMPT Wed May 9 13:24:55 CEST 2018
> >    CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
> >    CPU: VIVT data cache, VIVT instruction cache
> >    OF: fdt: Machine model: ARM Integrator/CP
> >    ...
> >    of-flash 24000000.flash: Integrator/CP flash protection
> >    of-flash 24000000.flash: do_map_probe() failed for type cfi_probe
> >    of-flash 24000000.flash: do_map_probe() failed
> >
> > Since we have a CFI pflash model available, wire it.
> > The kernel properly detects it:
> >
> >    of-flash 24000000.flash: Integrator/CP flash protection
> >    24000000.flash: Found 1 x32 devices at 0x0 in 32-bit bank. Manufacturer ID 
> > 0x000000 Chip ID 0x000000
> >    Intel/Sharp Extended Query Table at 0x0031
> >    Using buffer write method
> >
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > ---
> > v2: Kconfig change was not committed
> >
> > RFC because I have no idea of the flash model, its ID code, and which
> > default CFI family (1 or 2).
>
> ARM DUI 0102G ("ARM Firmware Suite Reference Guide") helpfully has
> a few details:
>
> Device                                  Size  Organization     Flash part
> Integrator/AP Boot flash               512KB  1x512K block     Atmel AT49LV040
> Integrator/AP Application flash         32MB  256x128K blocks  Intel 28F320S3
> Integrator/CP Boot/Application flash    16MB  64x256K blocks   Intel 28F640J3A
>
> (of which we only model the CP.) With luck that's enough to
> nail down the relevant device properties.

"Intel 28F640J3A" is everything we need, thanks!

> > @@ -646,6 +649,14 @@ static void integratorcp_init(MachineState *machine)
> >                             qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_CARDIN, 
> > 0));
> >       sysbus_create_varargs("pl041", 0x1d000000, pic[25], NULL);
> >
> > +    dinfo = drive_get(IF_PFLASH, 0, 0);
> > +    if (!pflash_cfi01_register(0x24000000, "pflash", 16 * MiB,
> > +                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> > +                               64 * KiB, 4, 0, 0, 0, 0, 0)) {
> > +        error_report("Error registering flash memory");
> > +        exit(1);
> > +    }
>
> Passing a 'width' argument of 0 means "weird legacy backcompat
> device that's a bad emulation of a pair of 16-bit devices";
> we should avoid that for new code, and instead set
> the width and device-width properties to whatever the
> hardware has. (This in turn means you can't use the old
> pflash_cfi01_register() function.)

OK I'll try to document that.

> Should we be using blk_by_legacy_dinfo() in new code?
> I'm not sure if there's a better way to do this if we don't
> need to maintain back-compat with old commandline specifications
> of the flash contents.
>
> thanks
> -- PMM