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 physcial 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
On Mon, 18 Feb 2019 at 13:07, Markus Armbruster <armbru@redhat.com> 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 physcial hardware, no luck. > > For now, adjust the byte size passed to match the actual size created, > and add a FIXME comment. I'm pretty sure that FLASH_SIZE here is supposed to be a byte count of the size of the pflash. That matches what Linux has in arch/sh/boards/mach-r2d/setup.c where it sets up the flash_resource struct. The r2dplus board is also I think known as RTS7751R2D. That takes us to https://elinux.org/RTS7751R2D_Handling_Manual (sadly the link to the "hardware manual" is broken). No idea what the block size would be. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 18 Feb 2019 at 13:07, Markus Armbruster <armbru@redhat.com> 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 physcial hardware, no luck.
>>
>> For now, adjust the byte size passed to match the actual size created,
>> and add a FIXME comment.
>
> I'm pretty sure that FLASH_SIZE here is supposed to be a
> byte count of the size of the pflash. That matches what
> Linux has in arch/sh/boards/mach-r2d/setup.c where it
> sets up the flash_resource struct.
Okay, that's some evidence for size 0x02000000 (32MiB).
However, we've created size (16 * KiB) * (FLASH_SIZE >> 16), i.e. 8MiB,
since at least commit 368a354f02b (v1.3.0), possibly since forever.
> The r2dplus board is also I think known as RTS7751R2D. That
> takes us to https://elinux.org/RTS7751R2D_Handling_Manual
> (sadly the link to the "hardware manual" is broken).
Quote section Flash ROM Mapping:
Currently, MTD device mapping on Flash ROM is set as below.
0x00000000-0x00020000 "bootloader"
0x00020000-0x00320000 "mtdblock1" XIP kernel
0x00320000-0x00520000 "mtdblock2"
0x00520000-0x01000000 "mtdblock3"
Suggests a size of 0x01000000 (16MiB). Now we have three candidates.
Pick one, any one, and I'll adjust my patch. All I really care about is
getting argument @size consistent with arguments @sector_len and
@nb_blocs, so I can ditch @nb_blocs in PATCH 09.
> No idea what the block size would be.
As long as that's the case, inertia wins by default.
On 2/19/19 4:45 PM, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Mon, 18 Feb 2019 at 13:07, Markus Armbruster <armbru@redhat.com> 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 physcial hardware, no luck.
"physical"
>>>
>>> For now, adjust the byte size passed to match the actual size created,
>>> and add a FIXME comment.
>>
>> I'm pretty sure that FLASH_SIZE here is supposed to be a
>> byte count of the size of the pflash. That matches what
>> Linux has in arch/sh/boards/mach-r2d/setup.c where it
>> sets up the flash_resource struct.
>
> Okay, that's some evidence for size 0x02000000 (32MiB).
>
> However, we've created size (16 * KiB) * (FLASH_SIZE >> 16), i.e. 8MiB,
> since at least commit 368a354f02b (v1.3.0), possibly since forever.
>
>> The r2dplus board is also I think known as RTS7751R2D. That
>> takes us to https://elinux.org/RTS7751R2D_Handling_Manual
>> (sadly the link to the "hardware manual" is broken).
>
> Quote section Flash ROM Mapping:
>
> Currently, MTD device mapping on Flash ROM is set as below.
> 0x00000000-0x00020000 "bootloader"
> 0x00020000-0x00320000 "mtdblock1" XIP kernel
> 0x00320000-0x00520000 "mtdblock2"
> 0x00520000-0x01000000 "mtdblock3"
>
> Suggests a size of 0x01000000 (16MiB). Now we have three candidates.
>
> Pick one, any one, and I'll adjust my patch. All I really care about is
> getting argument @size consistent with arguments @sector_len and
> @nb_blocs, so I can ditch @nb_blocs in PATCH 09.
>
>> No idea what the block size would be.
>
> As long as that's the case, inertia wins by default.
There is also a paper [*]:
The Renesas Technology R0P751RLC001RL (R2DPLUS) board was used
as our target device.
This board is often used to evaluate software for CE devices.
The specification is shown below.
CPU: SH7751R(SH4) 240Mhz
RAM: 64Mbyte
Compact flash: 512Mbyte
Flash ROM: 64Mbyte (32Mbyte available for root file system)
Let's use at least 16MB to be able to run the elinux cited kernel.
[*] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-125-134.pdf
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 2/19/19 4:45 PM, Markus Armbruster wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On Mon, 18 Feb 2019 at 13:07, Markus Armbruster <armbru@redhat.com> 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 physcial hardware, no luck. > > "physical" Thanks, will fix. >>>> >>>> For now, adjust the byte size passed to match the actual size created, >>>> and add a FIXME comment. >>> >>> I'm pretty sure that FLASH_SIZE here is supposed to be a >>> byte count of the size of the pflash. That matches what >>> Linux has in arch/sh/boards/mach-r2d/setup.c where it >>> sets up the flash_resource struct. >> >> Okay, that's some evidence for size 0x02000000 (32MiB). >> >> However, we've created size (16 * KiB) * (FLASH_SIZE >> 16), i.e. 8MiB, >> since at least commit 368a354f02b (v1.3.0), possibly since forever. >> >>> The r2dplus board is also I think known as RTS7751R2D. That >>> takes us to https://elinux.org/RTS7751R2D_Handling_Manual >>> (sadly the link to the "hardware manual" is broken). >> >> Quote section Flash ROM Mapping: >> >> Currently, MTD device mapping on Flash ROM is set as below. >> 0x00000000-0x00020000 "bootloader" >> 0x00020000-0x00320000 "mtdblock1" XIP kernel >> 0x00320000-0x00520000 "mtdblock2" >> 0x00520000-0x01000000 "mtdblock3" >> >> Suggests a size of 0x01000000 (16MiB). Now we have three candidates. >> >> Pick one, any one, and I'll adjust my patch. All I really care about is >> getting argument @size consistent with arguments @sector_len and >> @nb_blocs, so I can ditch @nb_blocs in PATCH 09. >> >>> No idea what the block size would be. >> >> As long as that's the case, inertia wins by default. > > There is also a paper [*]: > > The Renesas Technology R0P751RLC001RL (R2DPLUS) board was used > as our target device. > This board is often used to evaluate software for CE devices. > The specification is shown below. > CPU: SH7751R(SH4) 240Mhz > RAM: 64Mbyte > Compact flash: 512Mbyte > Flash ROM: 64Mbyte (32Mbyte available for root file system) Candidate #4: 64MiB. Bring 'em on! > > Let's use at least 16MB to be able to run the elinux cited kernel. > > [*] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-125-134.pdf That's a vote for changing the status quo (8 MiB). Perhaps Magnus, who maintains the machine, can pick a new value for us.
Hi guys, On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster <armbru@redhat.com> wrote: > > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > > > On 2/19/19 4:45 PM, Markus Armbruster wrote: > >> Peter Maydell <peter.maydell@linaro.org> writes: > >> > >>> On Mon, 18 Feb 2019 at 13:07, Markus Armbruster <armbru@redhat.com> 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 physcial hardware, no luck. > > > > "physical" > > Thanks, will fix. > > >>>> > >>>> For now, adjust the byte size passed to match the actual size created, > >>>> and add a FIXME comment. > >>> > >>> I'm pretty sure that FLASH_SIZE here is supposed to be a > >>> byte count of the size of the pflash. That matches what > >>> Linux has in arch/sh/boards/mach-r2d/setup.c where it > >>> sets up the flash_resource struct. > >> > >> Okay, that's some evidence for size 0x02000000 (32MiB). > >> > >> However, we've created size (16 * KiB) * (FLASH_SIZE >> 16), i.e. 8MiB, > >> since at least commit 368a354f02b (v1.3.0), possibly since forever. > >> > >>> The r2dplus board is also I think known as RTS7751R2D. That > >>> takes us to https://elinux.org/RTS7751R2D_Handling_Manual > >>> (sadly the link to the "hardware manual" is broken). > >> > >> Quote section Flash ROM Mapping: > >> > >> Currently, MTD device mapping on Flash ROM is set as below. > >> 0x00000000-0x00020000 "bootloader" > >> 0x00020000-0x00320000 "mtdblock1" XIP kernel > >> 0x00320000-0x00520000 "mtdblock2" > >> 0x00520000-0x01000000 "mtdblock3" > >> > >> Suggests a size of 0x01000000 (16MiB). Now we have three candidates. > >> > >> Pick one, any one, and I'll adjust my patch. All I really care about is > >> getting argument @size consistent with arguments @sector_len and > >> @nb_blocs, so I can ditch @nb_blocs in PATCH 09. > >> > >>> No idea what the block size would be. > >> > >> As long as that's the case, inertia wins by default. > > > > There is also a paper [*]: > > > > The Renesas Technology R0P751RLC001RL (R2DPLUS) board was used > > as our target device. > > This board is often used to evaluate software for CE devices. > > The specification is shown below. > > CPU: SH7751R(SH4) 240Mhz > > RAM: 64Mbyte > > Compact flash: 512Mbyte > > Flash ROM: 64Mbyte (32Mbyte available for root file system) > > Candidate #4: 64MiB. Bring 'em on! > > > > > Let's use at least 16MB to be able to run the elinux cited kernel. > > > > [*] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-125-134.pdf > > That's a vote for changing the status quo (8 MiB). > > 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, / magnus
Magnus Damm <magnus.damm@gmail.com> writes: > Hi guys, > > On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: >> >> > On 2/19/19 4:45 PM, Markus Armbruster wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >> >> >>> On Mon, 18 Feb 2019 at 13:07, Markus Armbruster <armbru@redhat.com> 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 physcial hardware, no luck. >> > >> > "physical" >> >> Thanks, will fix. >> >> >>>> >> >>>> For now, adjust the byte size passed to match the actual size created, >> >>>> and add a FIXME comment. >> >>> >> >>> I'm pretty sure that FLASH_SIZE here is supposed to be a >> >>> byte count of the size of the pflash. That matches what >> >>> Linux has in arch/sh/boards/mach-r2d/setup.c where it >> >>> sets up the flash_resource struct. >> >> >> >> Okay, that's some evidence for size 0x02000000 (32MiB). >> >> >> >> However, we've created size (16 * KiB) * (FLASH_SIZE >> 16), i.e. 8MiB, >> >> since at least commit 368a354f02b (v1.3.0), possibly since forever. >> >> >> >>> The r2dplus board is also I think known as RTS7751R2D. That >> >>> takes us to https://elinux.org/RTS7751R2D_Handling_Manual >> >>> (sadly the link to the "hardware manual" is broken). >> >> >> >> Quote section Flash ROM Mapping: >> >> >> >> Currently, MTD device mapping on Flash ROM is set as below. >> >> 0x00000000-0x00020000 "bootloader" >> >> 0x00020000-0x00320000 "mtdblock1" XIP kernel >> >> 0x00320000-0x00520000 "mtdblock2" >> >> 0x00520000-0x01000000 "mtdblock3" >> >> >> >> Suggests a size of 0x01000000 (16MiB). Now we have three candidates. >> >> >> >> Pick one, any one, and I'll adjust my patch. All I really care about is >> >> getting argument @size consistent with arguments @sector_len and >> >> @nb_blocs, so I can ditch @nb_blocs in PATCH 09. >> >> >> >>> No idea what the block size would be. >> >> >> >> As long as that's the case, inertia wins by default. >> > >> > There is also a paper [*]: >> > >> > The Renesas Technology R0P751RLC001RL (R2DPLUS) board was used >> > as our target device. >> > This board is often used to evaluate software for CE devices. >> > The specification is shown below. >> > CPU: SH7751R(SH4) 240Mhz >> > RAM: 64Mbyte >> > Compact flash: 512Mbyte >> > Flash ROM: 64Mbyte (32Mbyte available for root file system) >> >> Candidate #4: 64MiB. Bring 'em on! >> >> > >> > Let's use at least 16MB to be able to run the elinux cited kernel. >> > >> > [*] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-125-134.pdf >> >> That's a vote for changing the status quo (8 MiB). >> >> 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.
On 3/4/19 8:25 AM, Markus Armbruster wrote:
> Magnus Damm <magnus.damm@gmail.com> writes:
>> On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster <armbru@redhat.com> 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 :)
Regards,
Phil.
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 3/4/19 8:25 AM, Markus Armbruster wrote:
>> Magnus Damm <magnus.damm@gmail.com> writes:
>>> On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster <armbru@redhat.com> 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:
$ 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?
On 3/4/19 4:33 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> On 3/4/19 8:25 AM, Markus Armbruster wrote:
>>> Magnus Damm <magnus.damm@gmail.com> writes:
>>>> On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster <armbru@redhat.com> 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.
On Tue, 5 Mar 2019 at 17:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > But I'd recommend changing/fixing the sector size during the next dev > cycle, so we have more time for testing. Nobody in the upstream dev community is using or testing this board. The only way we'll find out if there's a problem with changing the sector size is to put the change in and get it into a release. I would vote for just making the change now. thanks -- PMM
On 3/5/19 6:25 PM, Peter Maydell wrote:
> On Tue, 5 Mar 2019 at 17:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> But I'd recommend changing/fixing the sector size during the next dev
>> cycle, so we have more time for testing.
>
> Nobody in the upstream dev community is using or testing this board.
Well I submitted a Avocado test last year:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02749.html
And I rebase/run it from time to time.
> The only way we'll find out if there's a problem with changing the
> sector size is to put the change in and get it into a release.
> I would vote for just making the change now.
I'm happy with this vote, and am sure Markus will be too :)
Markus: the last field I wasn't sure about without double checking the
code is the @width one. I find it misleading, is that the size of the
data bus or the size of the flash words? Answer: this is the size of the
words in byte. NOR flash devices can not write less data than their word
boundary.
The S29PL127J60TFI130 only support 16bit words, so using @width=2 is
correct.
And the winner is.... ta-da!
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, 2, 0x0001, 0x227e, 0x2220, 0x2200
0x555, 0x2aa, 0);
>
> thanks
> -- PMM
Thanks for your support!
Phil.
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 3/5/19 6:25 PM, Peter Maydell wrote: >> On Tue, 5 Mar 2019 at 17:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>> But I'd recommend changing/fixing the sector size during the next dev >>> cycle, so we have more time for testing. >> >> Nobody in the upstream dev community is using or testing this board. > > Well I submitted a Avocado test last year: > https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02749.html > > And I rebase/run it from time to time. > >> The only way we'll find out if there's a problem with changing the >> sector size is to put the change in and get it into a release. >> I would vote for just making the change now. > > I'm happy with this vote, and am sure Markus will be too :) > > Markus: the last field I wasn't sure about without double checking the > code is the @width one. I find it misleading, is that the size of the > data bus or the size of the flash words? Answer: this is the size of the > words in byte. NOR flash devices can not write less data than their word > boundary. > > The S29PL127J60TFI130 only support 16bit words, so using @width=2 is > correct. > > And the winner is.... ta-da! > > 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, 2, 0x0001, 0x227e, 0x2220, 0x2200 > 0x555, 0x2aa, 0); Sold! [...]
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 3/4/19 4:33 PM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: >>> On 3/4/19 8:25 AM, Markus Armbruster wrote: >>>> Magnus Damm <magnus.damm@gmail.com> writes: >>>>> On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster <armbru@redhat.com> 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 :/ And I fell for it, should've known better %-}
On 2/18/19 1:56 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 physcial 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, Good news: when you read (0x0000, 0x0000, 0x0000, 0x0000) pflash IDs, that means the code uses the "Virt PFlash". IOW this is not a physical model, since the guest obviously doesn't care about checking the flash model. The "VirtPFlash" only has 64KiB sectors. I suggest we add a pflash_cfi02_create_virt(reduced args) helper to make this obvious: pflash_cfi02_create_virt(paddr, name, size_bytes, mapping?).
On Tue, 19 Feb 2019 at 16:07, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 2/18/19 1:56 PM, Markus Armbruster wrote: > Good news: when you read (0x0000, 0x0000, 0x0000, 0x0000) pflash IDs, > that means the code uses the "Virt PFlash". IOW this is not a physical > model, since the guest obviously doesn't care about checking the flash > model. > The "VirtPFlash" only has 64KiB sectors. > > I suggest we add a pflash_cfi02_create_virt(reduced args) helper to make > this obvious: > > pflash_cfi02_create_virt(paddr, name, size_bytes, mapping?). What would this be, and when would you use it without a /* FIXME this is not what the real hardware does */ ? The real problem with most of these pflash creation calls is that they're using bogus data for the flash device (wrong IDs, wrong width, using the legacy weird implementation, etc etc) because nobody cares much about the boards or has real hardware to see what the hardware really is doing. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 19 Feb 2019 at 16:07, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 2/18/19 1:56 PM, Markus Armbruster wrote: >> Good news: when you read (0x0000, 0x0000, 0x0000, 0x0000) pflash IDs, >> that means the code uses the "Virt PFlash". Which code? >> IOW this is not a physical >> model, since the guest obviously doesn't care about checking the flash >> model. >> The "VirtPFlash" only has 64KiB sectors. >> >> I suggest we add a pflash_cfi02_create_virt(reduced args) helper to make >> this obvious: >> >> pflash_cfi02_create_virt(paddr, name, size_bytes, mapping?). > > What would this be, and when would you use it without a > /* FIXME this is not what the real hardware does */ ? For a purely virtual machine such as ARM virt, perhaps? Funnily, we use IDs 0x89, 0x18, 0x00, 0x00 there. Which first appeared in the microblaze petalogix-s3adsp1800 machine (commit 2548de3a343, Jan 2010), then got copied to ppc virtex-ml507 (commit 2c50e26efdb, Sep 2010), microblaze petalogix-ml605 (commit 00914b7d970, Mar 2011), ppc sam460ex (commit 4b387f9ee16, Feb 2018), arm vexpress-* in different form (commit b8433303fbc and 0163a2dc80, Dec 2013), and finally arm virt (commit acf82361c61, Sep 2014). Which ones match physical hardware is anybody's guess. > The real problem with most of these pflash creation calls > is that they're using bogus data for the flash device > (wrong IDs, wrong width, using the legacy weird implementation, > etc etc) Even better news: when we replace the helper function by longhand, many of the (bogus) qdev_prop_set_FOO() become no-ops (because the default is identically bogus), so we can sweep more of this under the carpet! > because nobody cares much about the boards or has > real hardware to see what the hardware really is doing. Remind me, why are we shipping stuff nobody cares much about?
On Tue, 19 Feb 2019 at 17:53, Markus Armbruster <armbru@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > What would this be, and when would you use it without a > > /* FIXME this is not what the real hardware does */ ? > > For a purely virtual machine such as ARM virt, perhaps? > > Funnily, we use IDs 0x89, 0x18, 0x00, 0x00 there. FWIW, this is "Manufacturer: Intel" and whatever device 0x18 is for Intel. IDs 2 and 3 are used by the pflash_cfi01 code only if we're using the bogus backward-compatibility "act like a bad emulation of two 16-bit devices making up a 32-bit wide device", in which case they're the ID values for the second of the two devices. (Or it might be that the ident values then are "manuf-id manuf-id device-id device-id", and with the not-bogus setup they're "manuf-id device-id unused unused".) I'm not sure whether the chips on any of these boards really are Intel ones :-) thanks -- PMM
© 2016 - 2026 Red Hat, Inc.