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

[Philippe Mathieu-Daudé]

On 3/4/19 4:33 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
>> On 3/4/19 8:25 AM, Markus Armbruster wrote:
>>> Magnus Damm <address@hidden> writes:
>>>> On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster <address@hidden> wrote:
>>>>> Perhaps Magnus, who maintains the machine, can pick a new value for us.
>>>>
>>>> According to the old board user document in Japanese (under NDA) what
>>>> is referred to as FROM (Area0) is connected via a 32-bit bus and CS0
>>>> to CN8. The docs mention s29pl127j60tfi130 but since I don't have the
>>>> board handy ATM I don't know how the chips are connected.
>>>>
>>>> Hope this helps,
>>>
>>> If you want me to change our emulated flash memory's size, please give
>>> me a number.
>>
>> datasheet "S29PL-J 002-00615 Rev. *E":
>> https://www.cypress.com/file/207091/download
>>
>> The S29PL127J60TFI130 is a 128Mbit NOR pflash addressable in words of 16bit.
>>
>> 128Mbit = 16 MiB
>>
>> At least it matches the "RTS7751R2D Handling Manual"!
>> https://elinux.org/RTS7751R2D_Handling_Manual#Kernel_start_from_FROM_extension_card_.28Kernel_space_XIP.29
>>
>> PL127J:
>> - 4 Banks
>>   -> we don't model banks.
>> - sectors of 4Kw and 32Kw
>>   -> we don't model different sector size and only use the
>>      biggest available
>>
>> sector_size = 32Kw = 64KiB // sector_len
>> (naive) sector_count = 256 // nb_blocs
>>
>> ManufID:  0001h
>> DeviceID: 227Eh 2220h 2200h
>>
>> I understand "connected via a 32-bit bus and CS0 to CN8" as the full
>> device wordsize is addressable, so this device
>>
>> So in pflash_cfi02_register() format:
>>
>> - name = "FROM (Area0)"
>> - size = 16 * MiB
>> - sector_len = 64 * KiB
>> - nb_blocs = 256
>> - nb_mappings = 1? /* Machine specific... */
>> - width = 2
>> - id0 = 0x0001
>> - id1 = 0x227e
>> - id2 = 0x2220
>> - id3 = 0x2200
>> - unlock_addr0 = 0x555,
>> - unlock_addr1 = 0x2aa
>> - be = 0 /* Arch specific... */
>>
>> Which hopefully is very similar to what we currently use :)
> 
> 'fraid not:

I was trying to be sarcastic :/

> 
>     $ qemu-system-sh4 -M r2d -display none -S -monitor stdio
>     QEMU 3.1.50 monitor - type 'help' for more information
>     (qemu) info qtree
>     [...]
>       dev: cfi.pflash02, id ""
>         drive = ""
>         num-blocks = 512 (0x200)        <--- instead of 256
>         sector-length = 16384 (0x4000)  <--- instead of 65536
>         width = 4 (0x4)                 <--- instead of 2
>         mappings = 1 (0x1)
>         big-endian = 0 (0x0)
>         id0 = 0 (0x0)                   <--- instead of 0x0001
>         id1 = 0 (0x0)                   <--- instead of 0x227e
>         id2 = 0 (0x0)                   <--- instead of 0x2220
>         id3 = 0 (0x0)                   <--- instead of 0x2200
>         unlock-addr0 = 1365 (0x555)
>         unlock-addr1 = 682 (0x2aa)
>         name = "r2d.flash"              <--- instead of "FROM (Area0)"
>         mmio 0000000000000000/0000000000800000
> 
> Not shown: size is num-blocks * sector-length = 512 * 16384 = 8MiB
> instead of 16MiB.
> 
> Which properties would you like me to change, and how?

The simpler change for correctness is to fix the FLASH_SIZE:

-#define FLASH_SIZE 0x02000000
+#define FLASH_SIZE (16 * MiB)

+   /*
+    * According to the old board user document in Japanese (under NDA) what
+    * is referred to as FROM (Area0) is connected via a 32-bit bus and CS0
+    * to CN8. The docs mention a Cypress S29PL127J60TFI130 chipsset.
+    * Per the 'S29PL-J 002-00615 Rev. *E' datasheet, it is a 128Mbit NOR
+    * parallel flash addressable in words of 16bit.
+    */
    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE,
                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                         16 * KiB, FLASH_SIZE >> 16,
+                         16 * KiB, FLASH_SIZE >> 14 /* will get removed
later */,
                          1, 4, 0x0000, 0x0000, 0x0000, 0x0000,
                          0x555, 0x2aa, 0);

                          1, 4, 0x0000, 0x0000, 0x0000, 0x0000,
                          0x555, 0x2aa, 0);

Now fixing the sector size:

    pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE,
                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                         16 * KiB, FLASH_SIZE >> 16,
-                         1, 4, 0x0000, 0x0000, 0x0000, 0x0000,
+                         64 * KiB, FLASH_SIZE >> 16 /* will get removed
later */,
+                         1, 4, 0x0001, 0x227e, 0x2220, 0x2200
                          0x555, 0x2aa, 0);

But I'd recommend changing/fixing the sector size during the next dev
cycle, so we have more time for testing.

Regards,

Phil.