Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/25683 )
Change subject: Add support for National Instruments USB-845x devices
......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/25683/8/flashrom.8.tmpl
File flashrom.8.tmpl:
https://review.coreboot.org/#/c/25683/8/flashrom.8.tmpl@1125
PS8, Line 1125:
kilo is always lowercase.
--
To view, visit https://review.coreboot.org/25683
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549
Gerrit-Change-Number: 25683
Gerrit-PatchSet: 10
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes HAOUAS <ehaouas(a)noos.fr>
Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com>
Gerrit-CC: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Comment-Date: Mon, 27 Aug 2018 00:07:32 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
David Hendricks has uploaded a new patch set (#22) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom)
......................................................................
Add support to get layout from fmap (e.g. coreboot rom)
Flashmap, or simply fmap, is a binary data format for describing
region offsets, sizes, and certain attributes and is widely used by
coreboot. This patch adds support for the fmap data format version 1.1
and adds --fmap and --fmap-file arguments.
Using --fmap will make flashrom to search the ROM content for fmap
data. Using --fmap-file will make flashrom search a supplied file
for fmap data.
An example of how to update the COREBOOT region of a ROM:
flashrom -p programmer --fmap -w coreboot.rom -i COREBOOT
flashrom -p programmer --fmap-file coreboot.rom -w coreboot.rom -i COREBOOT
The fmap functions are mostly copied from cbfstool.
Currently it is made mutually exclusive with other layout options until
we are more clever about this input.
Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
---
M Makefile
M cli_classic.c
M flashrom.8.tmpl
A fmap.c
A fmap.h
M libflashrom.c
M libflashrom.h
7 files changed, 643 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/23203/22
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 22
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
David Hendricks has uploaded a new patch set (#21) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom)
......................................................................
Add support to get layout from fmap (e.g. coreboot rom)
Flashmap, or simply fmap, is a binary data format for describing
region offsets, sizes, and certain attributes and is widely used by
coreboot. This patch adds support for the fmap data format version 1.1
and adds --fmap and --fmap-file arguments.
Using --fmap will make flashrom to search the ROM content for fmap
data. Using --fmap-file will make flashrom search a supplied file
for fmap data.
An example of how to update the COREBOOT region of a ROM:
flashrom -p programmer --fmap -w coreboot.rom -i COREBOOT
flashrom -p programmer --fmap-file coreboot.rom -w coreboot.rom -i COREBOOT
The fmap functions are mostly copied from cbfstool.
Currently it is made mutually exclusive with other layout options until
we are more clever about this input.
Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
---
M Makefile
M cli_classic.c
M flashrom.8.tmpl
A fmap.c
A fmap.h
M libflashrom.c
M libflashrom.h
7 files changed, 643 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/23203/21
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 21
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Nico Huber has posted comments on this change. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom)
......................................................................
Patch Set 19:
(2 comments)
https://review.coreboot.org/#/c/23203/19/cli_classic.c
File cli_classic.c:
https://review.coreboot.org/#/c/23203/19/cli_classic.c@619
PS19, Line 619: fill_flash->chip->total_size * 1024
> *nod* […]
I would go for flash_size(), keep things simple:
static inline size_t flash_size(const struct flashctx *);
https://review.coreboot.org/#/c/23203/19/fmap.c
File fmap.c:
https://review.coreboot.org/#/c/23203/19/fmap.c@62
PS19, Line 62: if (i == FMAP_STRLEN - 1) {
> I suspect the intention here is to say that if we reach FMAP_STRLEN - 1 without quitting due to the […]
What I meant was that I'd have written a similar condition below
the loop. A common pattern, FWIW, for-loop + check if it run all
the way through.
But you are right, just leave it as is.
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 19
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Fri, 24 Aug 2018 19:51:50 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Nico Huber has posted comments on this change. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom)
......................................................................
Patch Set 20:
(15 comments)
looks much better, thank you. I like the movement to fmap.c
https://review.coreboot.org/#/c/23203/20/cli_classic.c
File cli_classic.c:
https://review.coreboot.org/#/c/23203/20/cli_classic.c@107
PS20, Line 107: "r"
"rb" for windows? not sure
https://review.coreboot.org/#/c/23203/20/fmap.h
File fmap.h:
https://review.coreboot.org/#/c/23203/20/fmap.h@69
PS20, Line 69: const
`const` on the parameter itself is meaningless in the contract
https://review.coreboot.org/#/c/23203/19/fmap.h
File fmap.h:
https://review.coreboot.org/#/c/23203/19/fmap.h@33
PS19, Line 33: * GNU General Public License ("GPL") version 2 as published by the Free
> It's dual-licensed, as the last paragraph states. […]
That's why I though, why mention GPLv2 explicitly? Also wondering
if we miss something ;)
Ok checked, it's the standard 3-clause BSD text. Generally GPL
compatible... weird.
https://review.coreboot.org/#/c/23203/20/fmap.c
File fmap.c:
https://review.coreboot.org/#/c/23203/20/fmap.c@44
PS20, Line 44: int fmap_size(const struct fmap *fmap)
could be static now
https://review.coreboot.org/#/c/23203/20/fmap.c@47
PS20, Line 47: return -1;
All callers seem to know that it's non-NULL but ignore possible
negative return values. Maybe just remove the check and return a
size_t?
https://review.coreboot.org/#/c/23203/20/fmap.c@59
PS20, Line 59: flash
> What you actually check here is not the overall flashsize itself but the size of this FMAP instance […]
Are there nested FMAPs? I've only seen the default ones in coreboot
and they seem to cover the complete flash.
https://review.coreboot.org/#/c/23203/20/fmap.c@151
PS20, Line 151: off_t fmap_offset = fmap_lsearch(buf + rom_offset, len);
: if (fmap_offset < 0) {
: msg_gdbg("Failed to find fmap using linear search.\n");
: goto _free_ret;
: }
:
: int fmap_len = fmap_size((struct fmap *)&buf[fmap_offset]);
: *fmap_out = malloc(fmap_len);
: if (*fmap_out == NULL) {
: msg_gerr("Out of memory.\n");
: goto _free_ret;
: }
:
: memcpy(*fmap_out, buf + fmap_offset, fmap_len);
Beside the error message, this is probably the same as calling
fmap_read_from_buffer().
https://review.coreboot.org/#/c/23203/20/fmap.c@179
PS20, Line 179: unsigned int chip_size = flashctx->chip->total_size * 1024;
: int sig_len = strlen(FMAP_SIGNATURE);
make both const?
https://review.coreboot.org/#/c/23203/20/fmap.c@182
PS20, Line 182: if (!flashctx || !flashctx->chip)
: return 1;
> Would you like to add this check to fmap_lsearch_rom() as well?
Or just move it up into fmap_read_from_rom()?
https://review.coreboot.org/#/c/23203/20/fmap.c@256
PS20, Line 256: struct fmap *tmp = fmap;
: fmap = realloc(fmap, fmap_len);
: if (!fmap) {
: msg_gerr("Failed to realloc.\n");
: free(tmp);
: goto _free_ret;
: }
> realloc is allowed to move the re-allocated memory buffer to a completely new address[1]. […]
realloc() frees the original memory if moved. So we only have
to take care of the original pointer (tmp) in the error case,
what we do.
https://review.coreboot.org/#/c/23203/20/libflashrom.h
File libflashrom.h:
https://review.coreboot.org/#/c/23203/20/libflashrom.h@67
PS20, Line 67: const
the later const is not meaningful here
https://review.coreboot.org/#/c/23203/20/libflashrom.c
File libflashrom.c:
https://review.coreboot.org/#/c/23203/20/libflashrom.c@396
PS20, Line 396: if (l->num_entries == MAX_ROMLAYOUT) {
we could also check in advance `if (fmap->nareas > MAX_ROMLAYOUT)`
but this works too
https://review.coreboot.org/#/c/23203/20/libflashrom.c@405
PS20, Line 405: sizeof(l->entries[i].name)
FMAP_STRLEN is shorter, use that or MIN() of both?
Also an explicit 0-termination would be good (I know
there should be one in the FMAP, but we didn't check
the entries, did we?)
https://review.coreboot.org/#/c/23203/20/libflashrom.c@477
PS20, Line 477: ret = 1;
redundant
https://review.coreboot.org/#/c/23203/20/libflashrom.c@484
PS20, Line 484: ret = 1;
redundant
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 20
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Fri, 24 Aug 2018 19:41:01 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Nico Huber has posted comments on this change. ( https://review.coreboot.org/27444 )
Change subject: usbdev: Refactor device discovery code
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
I would drop the added `handle` check, but that's probably a matter of
taste :)
https://review.coreboot.org/#/c/27444/2/usbdev.c
File usbdev.c:
https://review.coreboot.org/#/c/27444/2/usbdev.c@84
PS2, Line 84:
> To follow up, the compiler won't let us do this. […]
I guess we could cast `filter_by_serial` in the call to
get_by_vid_pid_filter(). But it won't gain us much...
https://review.coreboot.org/#/c/27444/2/usbdev.c@105
PS2, Line 105:
> Fair point. […]
Actually the check makes it more confusing to me. If *nump == 0,
we want to stop filtering no matter if this is the first or second
call. One could say that we won't decrement *nump accidentally on
a call with handle != NULL... but if you think it that far, you
should already realize that the check is superfluous.
--
To view, visit https://review.coreboot.org/27444
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31ed572501e4314b9455e1b70a5e934ec96408b1
Gerrit-Change-Number: 27444
Gerrit-PatchSet: 4
Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org>
Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 24 Aug 2018 17:16:17 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Nico Huber has posted comments on this change. ( https://review.coreboot.org/27443 )
Change subject: usbdev: Extract libusb1 device discovery into a separate file
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/27443
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idfcc79371241c2c1dea97faf5e532aa971546a79
Gerrit-Change-Number: 27443
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 24 Aug 2018 17:02:14 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: Yes
Nico Huber has posted comments on this change. ( https://review.coreboot.org/28087 )
Change subject: Add initial J-Link SPI programmer
......................................................................
Patch Set 5:
Martin, any time now (the builder update for libjaylink) :)
--
To view, visit https://review.coreboot.org/28087
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e
Gerrit-Change-Number: 28087
Gerrit-PatchSet: 5
Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de>
Gerrit-Reviewer: Marc Schink <flashrom-dev(a)marcschink.de>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 24 Aug 2018 16:59:46 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
Nico Huber has posted comments on this change. ( https://review.coreboot.org/28087 )
Change subject: Add initial J-Link SPI programmer
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
I guess we are done here :)
https://review.coreboot.org/#/c/28087/2/jlink_spi.c
File jlink_spi.c:
https://review.coreboot.org/#/c/28087/2/jlink_spi.c@344
PS2, Line 344:
> I will add a comment to the documentation, thanks for the pointer ;)
heh, I only now realized that there's a zapb in the upstream
libjaylink URL ;)
--
To view, visit https://review.coreboot.org/28087
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e
Gerrit-Change-Number: 28087
Gerrit-PatchSet: 5
Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de>
Gerrit-Reviewer: Marc Schink <flashrom-dev(a)marcschink.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 24 Aug 2018 16:58:12 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes