[coreboot-gerrit] Change in coreboot[master]: [WIP]src/drivers/spi: Read Winbond's flash protection bits

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/25082 )

Change subject: [WIP]src/drivers/spi: Read Winbond's flash protection bits
......................................................................


Patch Set 1:

(10 comments)

https://review.coreboot.org/#/c/25082/1/src/drivers/spi/spi_flash.c
File src/drivers/spi/spi_flash.c:

https://review.coreboot.org/#/c/25082/1/src/drivers/spi/spi_flash.c@69
PS1, Line 69: printk(BIOS_WARNING, "SF: Failed to send command %02x: %d\n",
            : 		       cmd[0], ret);
Now that the command can contain more than one byte do you want to do something like this:
if (ret) {
	printk(BIOS_WARNING, "SF: Failed to send command ret: %d, cmd:", ret);
	for (i = 0; i < cmd_len; i++)
		printk(BIOS_WARNING, "%02x ", cmd[i]);
	printk(BIOS_WARNING, "\n");
}


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/spi_flash.c@438
PS1, Line 438: )
|| !region


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c
File src/drivers/spi/winbond.c:

https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@199
PS1, Line 199: port
part


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@204
PS1, Line 204: i
size_t i


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@219
PS1, Line 219: 1
use a macro for this?


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@233
PS1, Line 233: port
part


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@238
PS1, Line 238: block
size_t?


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@266
PS1, Line 266: KiB
Do you really need to divide by KiB?

block = reg.s.sec ? (4 * KiB) : max(64 * KiB, flash->size / 64);


https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@285
PS1, Line 285: 4
magic value. Do you want to add a macro for this?


https://review.coreboot.org/#/c/25082/1/src/include/spi_flash.h
File src/include/spi_flash.h:

https://review.coreboot.org/#/c/25082/1/src/include/spi_flash.h@107
PS1, Line 107: 0   if the device doesn't support block protection
             :  *   0   if the device doesn't enable block protection
             :  *   0
Do you think it would help if we returned different return codes for these caes?



-- 
To view, visit https://review.coreboot.org/25082
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I933a8abdc28174ec32acf323c102d606b58c1ea5
Gerrit-Change-Number: 25082
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph at 9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Furquan Shaikh <furquan at google.com>
Gerrit-Comment-Date: Fri, 09 Mar 2018 15:51:41 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180309/99a8adf9/attachment.html>