Open emard opened 3 years ago
Currently, enabling write protection on sectors is not supported. In fact it's opposite: if a sector is protected openFPGALoader unprotects this one... But this policy is, maybe, a bad idea.
Yes, I suggest changing that.
I expect it should refuse to write by default, requrie additional option like "--force" "--unprotect" or "--protect-override"
I'm trying to protect flash chip with external tools and check overwriting if protection "works" but tool already overrides it :)
On 10/4/21, Gwenhael Goavec-Merou @.***> wrote:
Currently, enabling write protection on sectors is not supported. In fact it's opposite: if a sector is protected openFPGALoader unprotects this one... But this policy is, maybe, a bad idea.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-933150897
I answer here for [https://github.com/trabucayre/openFPGALoader/issues/125#issuecomment-937586929].
I'm agree with your point of view. I see how to implement a first draft, no really clean but... I espect to push modifications at end of week-end. I'm a bit annoyed by breaking current behaviour (inconditional unlock) but it's make sense to add more security and don't do something without reporting to user. So my idea is
Great, this seems very good!
Well if there is a strong reason that current behaviour should keep because not to break some large scale scripting applications and similar, then I suggest option like --respect-protoection to warn and exit if protected, but generally would prefer that it respects by default
Depending on TB register, range of sectors either starts from 0 to some 2^n-1 value or from 2^n to end of flash. Some chips have range inversion (complement) bit CMP which makes oposite, instead of protected 0 - 2^n-1 it is unprotected 0 to 2^n-1 but protected 2^n to end, this way it can make less than half of chip unprotected and more than half protected
Protecting multiple individual sectors is not always supported with chips and I if user wants some different than range, it's too complex for command line, maybe a protection layout file but let's say we don't need so much detail about write protection
On 10/8/21, Gwenhael Goavec-Merou @.***> wrote:
I answer here for [https://github.com/trabucayre/openFPGALoader/issues/125#issuecomment-937586929].
I'm agree with your point of view. I see how to implement a first draft, no really clean but... I espect to push modifications at end of week-end. I'm a bit annoyed by breaking current behaviour (inconditional unlock) but it's make sense to add more security and don't do something without reporting to user. So my idea is
- 2 args
- unprotect (use to unlock sectors before write, lock after if a bitstream is provided, simply unlock when no bitstream)
- protect to lock a set of sectors (make sense to set tb = 1 to lock low sectors), I'm not sure to have to use start addr since lock always start at 0 (there is another unlock registers but it's not supported by all chips).
- when user ask to write protected sector without unprotect -> fails with a message explaining why and how to unlock
- a first draft for flash recognition based on jedec code: required to check if tb is OTP, and BPx offset (not always same place in status register).
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-938341794
Personnaly haven't script with autounlock required, and, I prefer too have a respect policy by default. Current behaviour is an error. A good message to explain why openFPGALoader can't write due to locked sectors make more sense.
TB bit seems the most common case, I think, in a first time, limit modification to use this one. For a more complex lock use case it's true it's maybe overkill for an FPGA purpose where it's not really required. If someone ask for this with a good explanation it's possible to modify this in future but for now it make sense to use the most common solution.
I have a first proof of concept, need to improve sector lock/unlock detection to accept or not write on a (un)locked area.
Yes, I agree that's a good behavior to respect policy. I also think it is good to have some knowledge of flash chip protection register and report what range is protected and what are values of protection registers with description what is the meaning for their bits and are they OTP or not.
I know 2 cases from practice. Some of ULX3S boards have ISSI IS25LP128, some Winbond W25Q128. For bootloader, first 2MB need to be protected.
To protect first 2MB, ISSI TB must be changed from factory default 0 to 1, TB cannot be reset back from 1 to 0 but generally 1 is what board needs and it will keep it.
For Winbond, register-2 comes with OTP bits CMP and IRL both factory default 0. For first 2MB protection this bits and register-2 should not be changed. If it is changed, there's no way back, and it's good to report to user at least that some changes are already done and can't be undone because of hard protection.
On 10/9/21, Gwenhael Goavec-Merou @.***> wrote:
Personnaly haven't script with autounlock required, and, I prefer too have a respect policy by default. Current behaviour is an error. A good message to explain why openFPGALoader can't write due to locked sectors make more sense.
TB bit seems the most common case, I think, in a first time, limit modification to use this one. For a more complex lock use case it's true it's maybe overkill for an FPGA purpose where it's not really required. If someone ask for this with a good explanation it's possible to modify this in future but for now it make sense to use the most common solution.
I have a first proof of concept, need to improve sector lock/unlock detection to accept or not write on a (un)locked area.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-939241531
I've started to push new methods to lock blocks, updated the struct to specify tb location (and added methods to erase 4/32/64 kb). Next push will be to unprotect block if required and allowed, and to relock after write. But I think, in a first time, I will push in a dedicated branch to check everything without breaking something.
Great, and I'd be happy to test!
On 10/20/21, Gwenhael Goavec-Merou @.***> wrote:
I've started to push new methods to lock blocks, updated the struct to specify tb location (and added methods to erase 4/32/64 kb). Next push will be to unprotect block if required and allowed, and to relock after write. But I think, in a first time, I will push in a dedicated branch to check everything without breaking something.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-947363179
Hi. I've pushed my draft in rework_spiflash branch. With these modifications
--unprotect-flash
is setI think this branch wiall be never merged in this state -> too many modifications in quite all class. A device_cfg
or something like this may be required to simplfy.
Currently I have to integrate
Great, I have tried it!
Winbond first 2MB is protected by value 0x30 Now it correctly refuses flashing and it even doesn't want to unprotect.
/tmp/openFPGALoader/build/openFPGALoader -b ulx3s --unprotect-flash -f ulx3s_12f_test.bit write to flash Jtag probe limited to 3MHz Jtag frequency : requested 6000000Hz -> real 3000000Hz ret 0 Enable configuration: DONE SRAM erase: DONE Open file DONE Parse file DONE Detected: Winbond W25Q128 256 sectors size: 128Mb Detected: Winbond W25Q128 256 sectors size: 128Mb RDSR : 30 WIP : 0 WEL : 0 BP : 4 TB : 1 SRWD : 0 00000000 00080000 30 unlock blocks Error: block protection is set can't unlock without --unprotect Error: Failed to program FPGA: memory locked
On 10/25/21, Gwenhael Goavec-Merou @.***> wrote:
Hi. I've pushed my draft in rework_spiflash branch. With these modifications
- if a flash is protected (unknown chip device) or if user wish to write into protected blocks flash will be updated only if
--unprotect-flash
is set- after a write if flash is protected, protection is reapplied
I think this branch wiall be never merged in this state -> too many modifications in quite all class. A
device_cfg
or something like this may be required to simplfy. Currently I have to integrate
- unlock blocks without write
- lock blocks
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-950543066
Weird. All my tests works fine. Need to find why it's not working... It demonstrate the need to have a more simple way to pass args like unprotect...
My flash chip is also wierd, I think once I set status register lock OTP to protect status register from changing but I'm not sure.
Chip actually can set register values, they can be stored and read effectively it never protects anything so it's kinda unbrickable
Old openFPGALoader normally writes to this chip
On 10/25/21, Gwenhael Goavec-Merou @.***> wrote:
Weird. All my tests works fine. Need to find why it's not working... It demonstrate the need to have a more simple way to pass args like unprotect...
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-950587273
I'm a dumb (and not enough cofee...) with a clenaup before pushing I've lost protect_flash
parameter to program
method...
Fixed.
I've pushed everything required to enable SPI flash protection.
now you have to use --protect-flash 0xABC
to protect are between 0 and len
I can confirm on that protection support works on ULX3S with W25Q128 and IS25LP128
Thanx a lot for this action!
On 10/26/21, Gwenhael Goavec-Merou @.***> wrote:
I've pushed everything required to enable SPI flash protection. now you have to use
--protect-flash 0xABC
to protect are between 0 and len-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-951590666
Great! Now it's time to do the same for all devices! Thanks
Additionally I have IS25LP032 which has buggy protection, it ignores TB bit and always top is protected, not following the same datasheet as IS25LP128 has which correctly works.
Maybe I have few other flash chip on other boards, For example Altera EPCS64 flash, which according to ID looks like rebranded winbond W25Q64
On 10/27/21, Gwenhael Goavec-Merou @.***> wrote:
Great! Now it's time to do the same for all devices! Thanks
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-952517541
a WA for this device needs to be done. Low address aren't protected but it's requires to do something for high address (CPU firmware). Maybe I must order one IS25LP032
I have IS25LP032 on some older ULX3S boards.
ECP5 bootloader must always be in first 2MB of flash. Although I have set OTP bit TB=1, still upper 2MB gets protected but not the lower 2MB which bootloader needs.
On 10/27/21, Gwenhael Goavec-Merou @.***> wrote:
a WA for this device needs to be done. Low address aren't protected but it's requires to do something for high address (CPU firmware). Maybe I must order one IS25LP032
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-952722798
Need to check if one boards I have has this chip. It's possible to order one on mouser but shipping cost are just overwhelming compared to flash price...
I will ask a friend to dig out old chip and send you. It is also possible that new releases of the chip will have bug fixed so I'd like you have old one
On 10/27/21, Gwenhael Goavec-Merou @.***> wrote:
Need to check if one boards I have has this chip. It's possible to order one on mouser but shipping cost are just overwhelming compared to flash price...
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-952858090
Thanks! I have to prepare a PCB for this device. If new releases are bugfree it's maybe interesting too: If it's possible to handle both and adapt behaviour it's better.
Standard 8-pin soic testing clip can grip it but we'll first see when we obtain it. I have only board with such chip. But we'll search to find something for you
https://www.ebay.com/itm/273261302080
On 10/28/21, Gwenhael Goavec-Merou @.***> wrote:
Thanks! I have to prepare a PCB for this device. If new releases are bugfree it's maybe interesting too: If it's possible to handle both and adapt behaviour it's better.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-953520102
Interesting tool! I think I will order one for this chip, but maybe others with different size. This avoid to spend time to design PCB :)
In some cases it can be clipped on powered board directly, I usually power them from same USB hub to keep common GND and avoid electrical discharge
with this clip flash can be scoped, sniffed and sometimes even flashed or ROM dumped
On 10/28/21, Gwenhael Goavec-Merou @.***> wrote:
Interesting tool! I think I will order one for this chip, but maybe others with different size. This avoid to spend time to design PCB :)
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-953579428
Definitely it's something I need in my toolbox :)
I've received my clip (but not currently tested). For de10nano, unfortunately the flash chip has a different package. Globally, after adding protect/unprotect into some FPGA's driver it appear a most common and generic method must be added to avoid a too big code dupplication.
OK, I will re-ask friend to find IS25LP032 chip which I think has bug with protection, top/bottom bit is ignored, always protecting top
On 11/6/21, Gwenhael Goavec-Merou @.***> wrote:
I've received my clip (but not currently tested). For de10nano, unfortunately the flash chip has a different package. Globally, after adding protect/unprotect into some FPGA's driver it appear a most common and generic method must be added to avoid a too big code dupplication.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-962417398
Thanks
HI
Friend found the ISSI 032 chip, normally we don't know will it have same bug I have seen. We need your shipping address. My mail is @.*** so you can send it to me and I'll coordinate with the friend to send the chip
On 11/6/21, Gwenhael Goavec-Merou @.***> wrote:
Thanks
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-962418036
I've updated openFPGALoader (branch review_spiflash
) with a first pass of refactoring. Next will be to refactor write, verify and dump, to have, more or less, only one piece of code for all devices.
HI, I compiled "rework_spiflash" branch. There's small bug in JTAG IDCODE, maybe only missing a digit. 0x21111043 is 12F 0x41111043 is 25F
12F: @.:/tmp/openFPGALoader/build$ fujprog -i ULX2S / ULX3S JTAG programmer v4.8 (git cc3ea93 built May 23 2021 17:03:05) Copyright (C) Marko Zec, EMARD, gojimmypi, kost and contributors Using USB cable: ULX3S FPGA 12K v3.0.8 FPGA IDCODE: 21111043 FPGA identified SIZE: 12 @.:/tmp/openFPGALoader/build$ ./openFPGALoader -b ulx3s --detect write to ram Jtag probe limited to 3MHz Jtag frequency : requested 6000000Hz -> real 3000000Hz ret 0 index 0: idcode 0x1111043 manufacturer lattice family ECP5 model LFE5U-12/25 irlength 8
25F: @.:/tmp/openFPGALoader/build$ fujprog -i ULX2S / ULX3S JTAG programmer v4.8 (git cc3ea93 built May 23 2021 17:03:05) Copyright (C) Marko Zec, EMARD, gojimmypi, kost and contributors Using USB cable: ULX3S FPGA 25K v2.1.2 FPGA IDCODE: 41111043 FPGA identified SIZE: 25 @.:/tmp/openFPGALoader/build$ ./openFPGALoader -b ulx3s --detect write to ram Jtag probe limited to 3MHz Jtag frequency : requested 6000000Hz -> real 3000000Hz ret 0 index 0: idcode 0x1111043 manufacturer lattice family ECP5 model LFE5U-12/25 irlength 8
On 11/19/21, Gwenhael Goavec-Merou @.***> wrote:
I've updated openFPGALoader (branch
review_spiflash
) with a first pass of refactoring. Next will be to refactor write, verify and dump, to have, more or less, only one piece of code for all devices.-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-973833562
It's not really a bug... 4 MSb are version in IEEE 1149. These bits are masked since issue #107 The funny thing is idcode is common for both 12 and 25. (see #114 ). This has no impact to load/program these devices.
12 and 25 should be internally the same, differing only in this IDCODE. Bitstream should also match full 32-bit idcode to be accepted by the chip.
There are also serdes chips, this is idcode example with similar difference bitstream acceptance is also similar, should match full 32-bit).
85F with serdes @.:/tmp/openFPGALoader/build$ fujprog -i ULX2S / ULX3S JTAG programmer v4.8 (git cc3ea93 built May 23 2021 17:03:05) Copyright (C) Marko Zec, EMARD, gojimmypi, kost and contributors Using USB cable: ULX3S FPGA 85K v3.0.3 FPGA IDCODE: 81113043 FPGA identified SIZE: 0 @.:/tmp/openFPGALoader/build$ ./openFPGALoader -b ulx3s --detect write to ram Jtag probe limited to 3MHz Jtag frequency : requested 6000000Hz -> real 3000000Hz ret 0 index 0: idcode 0x1113043 manufacturer lattice family ECP5 model LFE5U-85 irlength 8
85F ordinary @.:/tmp/openFPGALoader/build$ fujprog -i ULX2S / ULX3S JTAG programmer v4.8 (git cc3ea93 built May 23 2021 17:03:05) Copyright (C) Marko Zec, EMARD, gojimmypi, kost and contributors Using USB cable: ULX3S FPGA 85K v3.0.3 FPGA IDCODE: 41113043 FPGA identified SIZE: 85 @.:/tmp/openFPGALoader/build$ ./openFPGALoader -b ulx3s --detect write to ram Jtag probe limited to 3MHz Jtag frequency : requested 6000000Hz -> real 3000000Hz ret 0 index 0: idcode 0x1113043 manufacturer lattice family ECP5 model LFE5U-85 irlength 8
On 11/19/21, Gwenhael Goavec-Merou @.***> wrote:
It's not really a bug... 4 MSb are version in IEEE 1149. These bits are masked since issue #107 The funny thing is idcode is common for both 12 and 25. (see #114 ). This has no impact to load/program these devices.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-973861607
Exactly!
In fact the 24bits idcode is used at enumerate step (when openFPGALoader start). It's a convenient way to avoid to have to fill part.hpp will all possible revision for each FPGA. It's also possible to have access, using jtag instruction 0xE0
, the full 32bits idcode.
A match between full idcode and bitstream's idcode is required (#85) for all device (already done for gowin, TBD for all others). For lattice, I have the piece of code to find this into configuration data (.bit file) it's now only required to integrate this into lattice.cpp
class.
for ecp5 with some external commandline tools (ecpunpack/eppack) idcode can be changed to match onboard detected idcode.
If openFPGALoader would like to shell call here's the script that seamless & user-friendly changes ID to upload blink LED originally compiled for ordinary 85F into serdes 85F which as different ID and it will blink as well.
TMPBIT=/tmp/repacking_bitstream.config ecpunpack \ --idcode 0x41113043 \ ${1} ${TMPBIT} ecppack \ --idcode 0x81113043 \ --freq 62.0 \ ${TMPBIT} ${2} rm $TMPBIT
On 11/19/21, Gwenhael Goavec-Merou @.***> wrote:
Exactly! In fact the 24bits idcode is used at enumerate step (when openFPGALoader start). It's a convenient way to avoid to have to fill part.hpp will all possible revision for each FPGA. It's also possible to have access, using jtag instruction
0xE0
, the full 32bits idcode. A match between full idcode and bitstream's idcode is required (#85) for all device (already done for gowin, TBD for all others). For lattice, I have the piece of code to find this into configuration data (.bit file) it's now only required to integrate this intolattice.cpp
class.-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-974297282
It's interesting to see ability to repack bitstream with a different idcode, but I'm not sure if this task has to be done by openFPGALoader or manually (or automatically) previously the call to openFPGALoader.
If openFPGALoader distinguishes devices only by lowest 24-bit and ID doesn't match openFPGALoader can print message suggesting some shell commands how to unpack/repack to get matching ID.
On 11/19/21, Gwenhael Goavec-Merou @.***> wrote:
It's interesting to see ability to repack bitstream with a different idcode, but I'm not sure if this task has to be done by openFPGALoader or manually (or automatically) previously the call to openFPGALoader.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-974338281
openFPGALoader is able, at device driver level, to obtain the 32bits idcode -> it's possible to know full FPGA model. But it's true: an exact match must be done before loading/wrting bitstream and a message must be displayed when there is no match (with a link to doc/lattice.mk for instance). This script example may/must be added to lattice.md to provides a way to adapt bitstream according to the targeted model.
The IDCODE check is now applied in both master
and rework_spiflash
branches.
OK, I'm testing:
I have 12F connected and trying to upload 85F bitstream: Error: Failed to program FPGA: mismatch between target's idcode and bitstream idcode
This message is not easy to troubleshoot what should be done to succeed.
I'd like to have printed full 32-bit value of expected ID by the device and if possible to parse the bitstream, also the ID from the file bitstream.
On 11/20/21, Gwenhael Goavec-Merou @.***> wrote:
The IDCODE check is now applied in both
master
andrework_spiflash
branches.-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-974685546
I have updated with a better message (display FPGA and bitstream idcode). It's okay?
Almost perfect
Error: Failed to program FPGA: mismatch between target's idcode and bitstream idcode expected 0x41113043 got 0x21111043
I'd reword it more to be clearly hardware requires 0x41113043, bitstream has 0x21111043
On 11/20/21, Gwenhael Goavec-Merou @.***> wrote:
I have updated with a better message (display FPGA and bitstream idcode). It's okay?
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-974692183
Or sorry, it's actually reverse (file is for 85F, board is 12F :) message should be: bitstream has 0x41113043 hardware requires 0x21111043
On 11/20/21, D EMARD @.***> wrote:
Almost perfect
Error: Failed to program FPGA: mismatch between target's idcode and bitstream idcode expected 0x41113043 got 0x21111043
I'd reword it more to be clearly hardware requires 0x41113043, bitstream has 0x21111043
On 11/20/21, Gwenhael Goavec-Merou @.***> wrote:
I have updated with a better message (display FPGA and bitstream idcode). It's okay?
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-974692183
done. thanks
Yess that's it! Now it's completely clear what should be done
On 11/20/21, Gwenhael Goavec-Merou @.***> wrote:
done. thanks
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/trabucayre/openFPGALoader/issues/124#issuecomment-974697341
Great! Now I need to do the same thing for all others devices and file type:-) ... finishing spi flash refactoring and playing with the IS25LP032 ;-)
Thanks
I have more or less finished to refactor code. There is some case where it's not possible due to structure but globally it's seems good. Could you try again to check if there is no regression? Thanks
flash protection area has been improved to use top/bottom. A specific WA for IS25LP032 has been added too.
I've integrated everything into master branch. It's good for you? Thanks for motivation!
HI
Is there some support for write protection of flash chips, usually I need to protect bootloader in first 2MB of flash from accidental overwriting. My chips of interest are some ISSI and Winbond 16MB.