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

Philippe Mathieu-Daudé posted 2 patches 5 years, 8 months ago
[RFC PATCH v2 2/2] hw/arm/integratorcp: Map a CFI parallel flash
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago
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é <f4bug@amsat.org>
---
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).
---
 hw/arm/integratorcp.c | 11 +++++++++++
 hw/arm/Kconfig        |  1 +
 2 files changed, 12 insertions(+)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 59804140cd..40cedfd55a 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "cpu.h"
 #include "hw/sysbus.h"
@@ -24,6 +25,7 @@
 #include "hw/char/pl011.h"
 #include "hw/hw.h"
 #include "hw/irq.h"
+#include "hw/block/flash.h"
 
 #define TYPE_INTEGRATOR_CM "integrator_core"
 #define INTEGRATOR_CM(obj) \
@@ -589,6 +591,7 @@ static void integratorcp_init(MachineState *machine)
     MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
     qemu_irq pic[32];
     DeviceState *dev, *sic, *icp;
+    DriveInfo *dinfo;
     int i;
 
     cpuobj = object_new(machine->cpu_type);
@@ -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);
+    }
+
     if (nd_table[0].used)
         smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 61635f52c4..7f179f960f 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -73,6 +73,7 @@ config INTEGRATOR
     select PL050 # keyboard/mouse
     select PL110 # pl111 LCD controller
     select PL181 # display
+    select PFLASH_CFI01
     select SMC91C111
 
 config MAINSTONE
-- 
2.21.1


Re: [RFC PATCH v2 2/2] hw/arm/integratorcp: Map a CFI parallel flash
Posted by Peter Maydell 5 years, 8 months ago
On Sun, 23 Feb 2020 at 23:30, Philippe Mathieu-Daudé <f4bug@amsat.org> 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é <f4bug@amsat.org>
> ---
> 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.

> @@ -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.)

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

Re: [RFC PATCH v2 2/2] hw/arm/integratorcp: Map a CFI parallel flash
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago
On 2/25/20 1:47 PM, Peter Maydell wrote:
> On Sun, 23 Feb 2020 at 23:30, Philippe Mathieu-Daudé <f4bug@amsat.org> 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é <f4bug@amsat.org>
>> ---
>> 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
>