[v2] pflash: Fixes and cleanups

[Qemu-devel] [PATCH v2 08/11] r2d: Flash memory creation is confused about size, mark FIXME

Posted by Markus Armbruster 6 years, 8 months ago

pflash_cfi02_register() takes a size in bytes, a block size in bytes
and a number of blocks.  r2d_init() passes FLASH_SIZE, 16 * KiB,
FLASH_SIZE >> 16.  Does not compute: size doesn't match block size *
number of blocks.  The latter happens to win.  I tried to find
documentation on the physical hardware, no luck.

For now, adjust the byte size passed to match the actual size created,
and add a FIXME comment.

Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sh4/r2d.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index dcdb3728cb..ed18d1f351 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -290,7 +290,14 @@ static void r2d_init(MachineState *machine)
 
     /* onboard flash memory */
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE,
+    /*
+     * FIXME The code is confused about the size of the flash.  It
+     * used to pass FLASH_SIZE bytes, in FLASH_SIZE >> 16 blocks of
+     * 16KiB each, which does not compute, but creates one of
+     * FLASH_SIZE / 4 bytes anyway.  The current code does so too, but
+     * whether it's the right size is anybody's guess.
+     */
+    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE / 4,
                           dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                           16 * KiB, FLASH_SIZE >> 16,
                           1, 4, 0x0000, 0x0000, 0x0000, 0x0000,
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 08/11] r2d: Flash memory creation is confused about size, mark FIXME

Posted by Philippe Mathieu-Daudé 6 years, 8 months ago

On 2/26/19 8:34 PM, Markus Armbruster wrote:
> pflash_cfi02_register() takes a size in bytes, a block size in bytes
> and a number of blocks.  r2d_init() passes FLASH_SIZE, 16 * KiB,
> FLASH_SIZE >> 16.  Does not compute: size doesn't match block size *
> number of blocks.  The latter happens to win.  I tried to find
> documentation on the physical hardware, no luck.
> 
> For now, adjust the byte size passed to match the actual size created,
> and add a FIXME comment.
> 
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/sh4/r2d.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index dcdb3728cb..ed18d1f351 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -290,7 +290,14 @@ static void r2d_init(MachineState *machine)
>  
>      /* onboard flash memory */
>      dinfo = drive_get(IF_PFLASH, 0, 0);
> -    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE,
> +    /*
> +     * FIXME The code is confused about the size of the flash.  It
> +     * used to pass FLASH_SIZE bytes, in FLASH_SIZE >> 16 blocks of
> +     * 16KiB each, which does not compute, but creates one of
> +     * FLASH_SIZE / 4 bytes anyway.  The current code does so too, but
> +     * whether it's the right size is anybody's guess.
> +     */
> +    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE / 4,

NAck, please see suggestion on v1:
https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg01206.html

>                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>                            16 * KiB, FLASH_SIZE >> 16,
>                            1, 4, 0x0000, 0x0000, 0x0000, 0x0000,
>

Re: [Qemu-devel] [PATCH v2 08/11] r2d: Flash memory creation is confused about size, mark FIXME

Posted by Markus Armbruster 6 years, 8 months ago

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 2/26/19 8:34 PM, Markus Armbruster wrote:
>> pflash_cfi02_register() takes a size in bytes, a block size in bytes
>> and a number of blocks.  r2d_init() passes FLASH_SIZE, 16 * KiB,
>> FLASH_SIZE >> 16.  Does not compute: size doesn't match block size *
>> number of blocks.  The latter happens to win.  I tried to find
>> documentation on the physical hardware, no luck.
>> 
>> For now, adjust the byte size passed to match the actual size created,
>> and add a FIXME comment.
>> 
>> Cc: Magnus Damm <magnus.damm@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/sh4/r2d.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
>> index dcdb3728cb..ed18d1f351 100644
>> --- a/hw/sh4/r2d.c
>> +++ b/hw/sh4/r2d.c
>> @@ -290,7 +290,14 @@ static void r2d_init(MachineState *machine)
>>  
>>      /* onboard flash memory */
>>      dinfo = drive_get(IF_PFLASH, 0, 0);
>> -    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE,
>> +    /*
>> +     * FIXME The code is confused about the size of the flash.  It
>> +     * used to pass FLASH_SIZE bytes, in FLASH_SIZE >> 16 blocks of
>> +     * 16KiB each, which does not compute, but creates one of
>> +     * FLASH_SIZE / 4 bytes anyway.  The current code does so too, but
>> +     * whether it's the right size is anybody's guess.
>> +     */
>> +    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE / 4,
>
> NAck, please see suggestion on v1:
> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg01206.html

I'll use that to replace this patch in v3.  Thanks!

[...]