Anastasia Klm has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Factor out global state ......................................................................
it85spi.c: Factor out global state
Moves global state into spi_master data.
BUGS=b:172876667
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 71 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/1
diff --git a/it85spi.c b/it85spi.c index b998790..d6f39a4 100644 --- a/it85spi.c +++ b/it85spi.c @@ -73,11 +73,26 @@ #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4) #endif /* LPC_IO */
-#ifdef LPC_IO -static unsigned int shm_io_base; -#endif -static unsigned char *ce_high, *ce_low; -static int it85xx_scratch_rom_reenter = 0; +struct it85_data { + unsigned int shm_io_base; + unsigned char *ce_high, *ce_low; + int it85xx_scratch_rom_reenter; +}; + +static int it85xx_spi_send_command(const struct flashctx *flash, + unsigned int writecnt, unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr); + +static struct spi_master spi_master_it85xx = { + .max_data_read = 64, + .max_data_write = 64, + .command = it85xx_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +};
/* This function will poll the keyboard status register until either * an expected value shows up, or the timeout is reached. @@ -106,12 +121,12 @@
/* IT8502 employs a scratch RAM when flash is being updated. Call the following * two functions before/after flash erase/program. */ -static void it85xx_enter_scratch_rom(void) +static void it85xx_enter_scratch_rom(struct it85_data *data) { int ret, tries;
msg_pdbg("%s():%d was called ...\n", __func__, __LINE__); - if (it85xx_scratch_rom_reenter > 0) + if (data->it85xx_scratch_rom_reenter > 0) return;
#if 0 @@ -156,14 +171,14 @@
if (tries < MAX_TRY) { /* EC already runs on SRAM */ - it85xx_scratch_rom_reenter++; + data->it85xx_scratch_rom_reenter++; msg_pdbg("%s():%d * SUCCESS.\n", __func__, __LINE__); } else { msg_perr("%s():%d * Max try reached.\n", __func__, __LINE__); } }
-static void it85xx_exit_scratch_rom(void) +static void it85xx_exit_scratch_rom(struct it85_data *data) { #if 0 int ret; @@ -171,7 +186,7 @@ int tries;
msg_pdbg("%s():%d was called ...\n", __func__, __LINE__); - if (it85xx_scratch_rom_reenter <= 0) + if (data->it85xx_scratch_rom_reenter <= 0) return;
for (tries = 0; tries < MAX_TRY; ++tries) { @@ -199,7 +214,7 @@ }
if (tries < MAX_TRY) { - it85xx_scratch_rom_reenter = 0; + data->it85xx_scratch_rom_reenter = 0; msg_pdbg("%s():%d * SUCCESS.\n", __func__, __LINE__); } else { msg_perr("%s():%d * Max try reached.\n", __func__, __LINE__); @@ -217,8 +232,10 @@
static int it85xx_shutdown(void *data) { + struct it85_data *drv_data = (struct it85_data *)data; + msg_pdbg("%s():%d\n", __func__, __LINE__); - it85xx_exit_scratch_rom(); + it85xx_exit_scratch_rom(drv_data);
return 0; /* FIXME: Should probably return something meaningful */ } @@ -227,31 +244,41 @@ { chipaddr base;
+ struct it85_data *data = calloc(1, sizeof(struct it85_data)); + if (!data) { + msg_perr("Out of memory!\n"); + return 1; + } + data->it85xx_scratch_rom_reenter = 0; + spi_master_it85xx.data = data; + msg_pdbg("%s():%d superio.vendor=0x%02x\n", __func__, __LINE__, s.vendor);
- if (register_shutdown(it85xx_shutdown, NULL)) + if (register_shutdown(it85xx_shutdown, data)) { + free(data); return 1; + }
#ifdef LPC_IO /* Get LPCPNP of SHM. That's big-endian. */ sio_write(s.port, LDNSEL, 0x0F); /* Set LDN to SHM (0x0F) */ - shm_io_base = (sio_read(s.port, SHM_IO_BAR0) << 8) + + data->shm_io_base = (sio_read(s.port, SHM_IO_BAR0) << 8) + sio_read(s.port, SHM_IO_BAR1); - msg_pdbg("%s():%d shm_io_base=0x%04x\n", __func__, __LINE__, - shm_io_base); + msg_pdbg("%s():%d it85_data->shm_io_base=0x%04x\n", __func__, __LINE__, + data->shm_io_base);
/* These pointers are not used directly. They will be send to EC's * register for indirect access. */ base = 0xFFFFF000; - ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ - ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */ + data->ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ + data->ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */
/* pre-set indirect-access registers since in most of cases they are * 0xFFFFxx00. */ - INDIRECT_A0(shm_io_base, base & 0xFF); - INDIRECT_A2(shm_io_base, (base >> 16) & 0xFF); - INDIRECT_A3(shm_io_base, (base >> 24)); + INDIRECT_A0(data->shm_io_base, base & 0xFF); + INDIRECT_A2(data->shm_io_base, (base >> 16) & 0xFF); + INDIRECT_A3(data->shm_io_base, (base >> 24)); #endif #ifdef LPC_MEMORY /* FIXME: We should block accessing that region for anything else. @@ -263,28 +290,13 @@
msg_pdbg("%s():%d base=0x%08x\n", __func__, __LINE__, (unsigned int)base); - ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ - ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ + data->ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ + data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ #endif
return 0; }
-static int it85xx_spi_send_command(const struct flashctx *flash, - unsigned int writecnt, unsigned int readcnt, - const unsigned char *writearr, - unsigned char *readarr); - -static const struct spi_master spi_master_it85xx = { - .max_data_read = 64, - .max_data_write = 64, - .command = it85xx_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, -}; - int it85xx_spi_init(struct superio s) { int ret; @@ -328,42 +340,51 @@ unsigned char *readarr) { unsigned int i; + if (!(flash->mst)) { + msg_perr("No data in flash context!\n"); + return 1; + } + struct it85_data *data = (struct it85_data *) flash->mst->spi.data; + if (!data) { + msg_perr("No data in flash context!\n"); + return 1; + }
- it85xx_enter_scratch_rom(); + it85xx_enter_scratch_rom(data); /* Exit scratch ROM ONLY when programmer shuts down. Otherwise, the * temporary flash state may halt the EC. */
#ifdef LPC_IO - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); - INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_low) >> 8) & 0xff); + INDIRECT_A1(data->shm_io_base, (((unsigned long int)data->ce_high) >> 8) & 0xff); + INDIRECT_WRITE(data->shm_io_base, 0xFF); /* Write anything to this address.*/ + INDIRECT_A1(data->shm_io_base, (((unsigned long int)data->ce_low) >> 8) & 0xff); #endif #ifdef LPC_MEMORY - mmio_writeb(0, ce_high); + mmio_writeb(0, data->ce_high); #endif for (i = 0; i < writecnt; ++i) { #ifdef LPC_IO - INDIRECT_WRITE(shm_io_base, writearr[i]); + INDIRECT_WRITE(data->shm_io_base, writearr[i]); #endif #ifdef LPC_MEMORY - mmio_writeb(writearr[i], ce_low); + mmio_writeb(writearr[i], data->ce_low); #endif } for (i = 0; i < readcnt; ++i) { #ifdef LPC_IO - readarr[i] = INDIRECT_READ(shm_io_base); + readarr[i] = INDIRECT_READ(data->shm_io_base); #endif #ifdef LPC_MEMORY - readarr[i] = mmio_readb(ce_low); + readarr[i] = mmio_readb(data->ce_low); #endif } #ifdef LPC_IO - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); - INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ + INDIRECT_A1(data->shm_io_base, (((unsigned long int)data->ce_high) >> 8) & 0xff); + INDIRECT_WRITE(data->shm_io_base, 0xFF); /* Write anything to this address.*/ #endif #ifdef LPC_MEMORY - mmio_writeb(0, ce_high); + mmio_writeb(0, data->ce_high); #endif
return 0;
Anastasia Klm has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Factor out global state ......................................................................
Patch Set 1:
thank you!
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Factor out global state ......................................................................
Patch Set 1:
(13 comments)
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@7 PS1, Line 7: Factor out global state Refactor singleton states into reentrant pattern
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@9 PS1, Line 9: Moves global state into spi_master data. Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@11 PS1, Line 11: BUGS BUG=b:172876667 TEST=builds
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@a273 PS1, Line 273: static int it85xx_spi_send_command(const struct flashctx *flash, : unsigned int writecnt, unsigned int readcnt, : const unsigned char *writearr, : unsigned char *readarr); this forward declaration can be deleted if in a patch before this one - you will need to move the it85xx_spi_send_command() function before the `struct spi_master spi_master_it85xx` declaration.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@a278 PS1, Line 278: static const struct spi_master spi_master_it85xx = { : .max_data_read = 64, : .max_data_write = 64, : .command = it85xx_spi_send_command, : .multicommand = default_spi_send_multicommand, : .read = default_spi_read, : .write_256 = default_spi_write_256, : .write_aai = default_spi_write_aai, : }; : keep this here.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@87 PS1, Line 87: static struct spi_master spi_master_it85xx = { : .max_data_read = 64, : .max_data_write = 64, : .command = it85xx_spi_send_command, : .multicommand = default_spi_send_multicommand, : .read = default_spi_read, : .write_256 = default_spi_write_256, : .write_aai = default_spi_write_aai, : }; actually this shouldn't move
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@235 PS1, Line 235: struct it85_data *drv_data = (struct it85_data *)data; calling it85xx_exit_scratch_rom(data) should auto cast the void * so no need for this line.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@235 PS1, Line 235: struct it85_data *drv_data = (struct it85_data *)data; calling it85xx_exit_scratch_rom(data) should auto cast the void * so no need for this line.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@238 PS1, Line 238: _rom(drv_data); after this line you should free data
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@248 PS1, Line 248: if (!data) { : msg_perr("Out of memory!\n"); : return 1; : } ``` if (!data) { msg_perr("Unable to allocate space for extra SPI master data.\n"); return SPI_GENERIC_ERROR; } ```
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@258 PS1, Line 258: if (register_shutdown(it85xx_shutdown, data)) { : free(data); : return 1; : } move to function eulogy.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@309 PS1, Line 309: msg_pdbg("FWH: %s():%d ret=%d\n", __func__, __LINE__, ret); : if (!ret) { as a patch before this one perhaps remove this debug line and invert the if logic to take the rest of the function out of the if scope and instead return early on failure i.e., `if (ret) return ret;`
and at the end of the function return 0;
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@343 PS1, Line 343: if (!(flash->mst)) { : msg_perr("No data in flash context!\n"); : return 1; : } you do not need this validation as it occurs already in the core. The dispatch site will always provide a valid mst pointer address.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Factor out global state ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@238 PS1, Line 238: _rom(drv_data);
after this line you should free data
After looking at this a bit more, I think you may as well create a patch that removes some of these largely useless indirection.
Part 1/2) remove this function and replace the call site with a direct call to `it85xx_exit_scratch_rom()`.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@309 PS1, Line 309: msg_pdbg("FWH: %s():%d ret=%d\n", __func__, __LINE__, ret); : if (!ret) {
as a patch before this one perhaps remove this debug line and invert the if logic to take the rest o […]
Part 2/2) Inline it85xx_spi_common_init() implementation here.
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#2).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 62 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/2
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@7 PS1, Line 7: Factor out global state
Refactor singleton states into reentrant pattern
Done
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@9 PS1, Line 9: Moves global state into spi_master data.
Move global singleton states into a struct and store within the spi_master data field for the life-t […]
Done
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@11 PS1, Line 11: BUGS
BUG=b:172876667 […]
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@87 PS1, Line 87: static struct spi_master spi_master_it85xx = { : .max_data_read = 64, : .max_data_write = 64, : .command = it85xx_spi_send_command, : .multicommand = default_spi_send_multicommand, : .read = default_spi_read, : .write_256 = default_spi_write_256, : .write_aai = default_spi_write_aai, : };
actually this shouldn't move
I moved it as we discussed, it is now in the middle (no need to be on the very top).
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@235 PS1, Line 235: struct it85_data *drv_data = (struct it85_data *)data;
calling it85xx_exit_scratch_rom(data) should auto cast the void * so no need for this line.
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@235 PS1, Line 235: struct it85_data *drv_data = (struct it85_data *)data;
calling it85xx_exit_scratch_rom(data) should auto cast the void * so no need for this line.
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@343 PS1, Line 343: if (!(flash->mst)) { : msg_perr("No data in flash context!\n"); : return 1; : }
you do not need this validation as it occurs already in the core. […]
I wrote get_data_from_context taking it87spi.c as an inspiration.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 2: Code-Review+1
(4 comments)
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@76 PS2, Line 76: it85_data call this `it85spi_data`
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@222 PS2, Line 222: : return 0 you need to `free(data);` before the return since it is a heap allocation.
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@312 PS2, Line 312: msg_perr("Out of memory!\n"); : return 1; : } ``` msg_perr("Unable to allocate space for extra SPI master data.\n"); return SPI_GENERIC_ERROR; } ```
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@315 PS2, Line 315: data->it85xx_scratch_rom_reenter = 0; I know the original code doesn't do it but its probably worth initialising the rest as well.
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#3).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 66 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/3
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#4).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 66 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/4
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 4:
(10 comments)
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@a273 PS1, Line 273: static int it85xx_spi_send_command(const struct flashctx *flash, : unsigned int writecnt, unsigned int readcnt, : const unsigned char *writearr, : unsigned char *readarr);
this forward declaration can be deleted if in a patch before this one - you will need to move the it […]
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@a278 PS1, Line 278: static const struct spi_master spi_master_it85xx = { : .max_data_read = 64, : .max_data_write = 64, : .command = it85xx_spi_send_command, : .multicommand = default_spi_send_multicommand, : .read = default_spi_read, : .write_256 = default_spi_write_256, : .write_aai = default_spi_write_aai, : }; :
keep this here.
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@87 PS1, Line 87: static struct spi_master spi_master_it85xx = { : .max_data_read = 64, : .max_data_write = 64, : .command = it85xx_spi_send_command, : .multicommand = default_spi_send_multicommand, : .read = default_spi_read, : .write_256 = default_spi_write_256, : .write_aai = default_spi_write_aai, : };
I moved it as we discussed, it is now in the middle (no need to be on the very top).
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@248 PS1, Line 248: if (!data) { : msg_perr("Out of memory!\n"); : return 1; : }
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@258 PS1, Line 258: if (register_shutdown(it85xx_shutdown, data)) { : free(data); : return 1; : }
move to function eulogy.
Done
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@343 PS1, Line 343: if (!(flash->mst)) { : msg_perr("No data in flash context!\n"); : return 1; : }
I wrote get_data_from_context taking it87spi.c as an inspiration.
Done
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@76 PS2, Line 76: it85_data
call this `it85spi_data`
Done
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@222 PS2, Line 222: : return 0
you need to `free(data);` before the return since it is a heap allocation.
Done
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@312 PS2, Line 312: msg_perr("Out of memory!\n"); : return 1; : }
Done
https://review.coreboot.org/c/flashrom/+/47655/2/it85spi.c@315 PS2, Line 315: data->it85xx_scratch_rom_reenter = 0;
I know the original code doesn't do it but its probably worth initialising the rest as well.
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c@250 PS4, Line 250: unsigned int i; : : struct it85spi_data *data; keep the declarations together. So
``` unsigned int i; struct it85spi_data *data;
if (get_data_from_context(flash, &data) < 0) { .. ```
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c@318 PS4, Line 318: data->ce_high = 0; : data->ce_low = 0; ``` data->ce_high = NULL; data->ce_low = NULL; ```
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c@326 PS4, Line 326: { : return 1; `free(data);` before the return, it was correct before.
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#5).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 67 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/5
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c@250 PS4, Line 250: unsigned int i; : : struct it85spi_data *data;
keep the declarations together. So […]
Done
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c@318 PS4, Line 318: data->ce_high = 0; : data->ce_low = 0;
Done
https://review.coreboot.org/c/flashrom/+/47655/4/it85spi.c@326 PS4, Line 326: { : return 1;
`free(data);` before the return, it was correct before.
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5: Code-Review+2
Eizan Miyamoto has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@a76 PS5, Line 76: #ifdef LPC_IO : static unsigned int shm_io_base; : #endif : to satisfy my curiosity, why did we get rid of this preprocessor stuff?
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@311 PS5, Line 311: calloc nit: Maybe to make it super-obvious, add a comment that the free() of data is handled inside it85xx_shutdown whose call is registered by register_shutdown() below?
Eizan Miyamoto has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@311 PS5, Line 311: calloc
nit: Maybe to make it super-obvious, add a comment that the free() of data is handled inside it85xx_ […]
from out-of-band comms, it's idiomatic in this codebase to assume that things will be deinitialized by the presence of the call to register_shutdown.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@a76 PS5, Line 76: #ifdef LPC_IO : static unsigned int shm_io_base; : #endif :
to satisfy my curiosity, why did we get rid of this preprocessor stuff?
I would say, this preprocessor stuff is not needed anymore. Prior to this, not using shm_io_base would produce compiler warning. However now shm_io_base is a member of a struct and compiler does not warn if the individual member of the struct is unused as long as struct is used.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@238 PS1, Line 238: _rom(drv_data);
After looking at this a bit more, I think you may as well create a patch that removes some of these […]
As discussed, this can go into a later patch.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@309 PS1, Line 309: msg_pdbg("FWH: %s():%d ret=%d\n", __func__, __LINE__, ret); : if (!ret) {
Part 2/2) Inline it85xx_spi_common_init() implementation here.
As discussed, this can go into a later patch.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@76 PS5, Line 76: it85spi_data nit: _data is a bit redundant and verbose; It might be cleaner and more direct to just use: `struct it85spi` here, and later, in code `struct it85spi it85spi;`
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@77 PS5, Line 77: unsigned int shm_io_base; : unsigned char *ce_high, *ce_low; : int it85xx_scratch_rom_reenter; : nit: probably want tab indents here in struct definition and for all the code below. I recommend spending some time figuring out how to set up your code editor to follow the style guide for this code base which uses tabs. I'm a bit surprised this isn't picked up by a linter on upload?
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@227 PS5, Line 227: static int get_data_from_context(const struct flashctx *flash, struct it85spi_data **data) : { : if (!flash || !flash->mst || !flash->mst->spi.data) { : msg_perr("Unable to extract data from flash context.\n"); : return SPI_GENERIC_ERROR; : } : *data = (struct it85spi_data *) flash->mst->spi.data; : : return 0; : } : Perhaps just return NULL on error:
static struct it85spi_data *get_data_from_context(const struct flashctx *flash) { if (!flash || !flash->mst || !flash->mst->spi.data) { msg_perr("Unable to extract data from flash context.\n"); return NULL; } return flash->mst->spi.data; }
And then below:
data = get_data_from_context(flash); if (!data) return SPI_GENERIC_ERROR;
But really, since this is the only place where we extract data from flash, probably no need for this separate function, and just inline the intermediate flash NULL checks.... and further, is it even possible for it85xx_spi_send_command() to be called with a NULL flash or flash->mst? If not, keep the code simple and just let it crash here?
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@321 PS5, Line 321: spi_master_it85xx.data = data; If spi_master_it85xx:it85spi_data is 1:1, and spi_master_it85xx is a static global, then there doesn't seem to be any reason why it85spi_data should be allocated at runtime. Just define it85spi_data a static global as well and skip the calloc / free and associated complexity. Perhaps something like:
static struct it85spi it85spi; static const struct spi_master spi_master_it85xx = { ... .data = &it85spi; };
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@76 PS5, Line 76: it85spi_data
nit: _data is a bit redundant and verbose; It might be cleaner and more direct to just use: `struct […]
This is consistent with the rest of the tree, please keep things consistent and we can argue about the merits of better identifier conventions after everything has converged to some consistency first.
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@77 PS5, Line 77: unsigned int shm_io_base; : unsigned char *ce_high, *ce_low; : int it85xx_scratch_rom_reenter; :
nit: probably want tab indents here in struct definition and for all the code below. […]
thanks for picking this up.
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@227 PS5, Line 227: static int get_data_from_context(const struct flashctx *flash, struct it85spi_data **data) : { : if (!flash || !flash->mst || !flash->mst->spi.data) { : msg_perr("Unable to extract data from flash context.\n"); : return SPI_GENERIC_ERROR; : } : *data = (struct it85spi_data *) flash->mst->spi.data; : : return 0; : } :
Perhaps just return NULL on error: […]
While true that this could be inlined it would be inconsistent with other drivers. Further, the helper is useful for writing unit-tests as the setup of flashromctx can be separated pre-spi-driver runs. This allows for driver state injection during unit-testing. The internal states can also be inspected using these helpers to constrain the test by way of type-signature and hence then the common return type of int was preserved. The consistent pattern here is one state tracker type and one getter from the `void *data` field.
The over arching vision is to first bring the tree into a consistent state where all drivers are fully re-enterent and have common type-siguratures that bond logic problems into smaller state spaces. Finally, to be consistent across the entire tree so that more smaller optimisations can be made atomically at once rather than a fragmented mess of different ways that troubles us now.
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@321 PS5, Line 321: spi_master_it85xx.data = data;
If spi_master_it85xx:it85spi_data is 1:1, and spi_master_it85xx is a static global, then there doesn […]
We explicitly do not want static globals of any kind though, that would be counter to the entire patch. Further to that, this is inconsistent with the rest of the trees direction.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@76 PS5, Line 76: it85spi_data
This is consistent with the rest of the tree, please keep things consistent and we can argue about t […]
sgtm. Consistency is best.
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@227 PS5, Line 227: static int get_data_from_context(const struct flashctx *flash, struct it85spi_data **data) : { : if (!flash || !flash->mst || !flash->mst->spi.data) { : msg_perr("Unable to extract data from flash context.\n"); : return SPI_GENERIC_ERROR; : } : *data = (struct it85spi_data *) flash->mst->spi.data; : : return 0; : } :
While true that this could be inlined it would be inconsistent with other drivers. […]
Consistency is good. Better would be to implement the unit tests in this CL that show the value in adding this extra complexity. Otherwise I still recommend the simpler and direct implementation for now, and then adding this complexity when there is value in doing so.
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@321 PS5, Line 321: spi_master_it85xx.data = data;
We explicitly do not want static globals of any kind though, that would be counter to the entire pat […]
Removing "static globals of any kind" is not a goal in and of itself. They are quite useful and efficient. There has to be some motivation to do so, such as when runtime allocation provides some value.
The value of this patch is that it collects scattered device specific file-global static state variables into a single context structure, and then makes the functions pure by having them operate on a passed-in context struct rather than directly accessing global state.
I don't see any additional value, at this point, of allocating the context struct on the heap rather than as a static global. AFAICT, it just introduces extra runtime complexity.
Perhaps the value is realized by some other patches that perform additional refactoring and/or add unit tests?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@227 PS5, Line 227: static int get_data_from_context(const struct flashctx *flash, struct it85spi_data **data) : { : if (!flash || !flash->mst || !flash->mst->spi.data) { : msg_perr("Unable to extract data from flash context.\n"); : return SPI_GENERIC_ERROR; : } : *data = (struct it85spi_data *) flash->mst->spi.data; : : return 0; : } :
Consistency is good. […]
In that case, I think inlining in this patch seems reasonable to keep this commit diff down some.
Hello Eizan Miyamoto, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#6).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 59 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/6
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 6: Code-Review+2
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 6:
(2 comments)
Thank you everyone for your comments!
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@77 PS5, Line 77: unsigned int shm_io_base; : unsigned char *ce_high, *ce_low; : int it85xx_scratch_rom_reenter; :
thanks for picking this up.
Done, I set up editor so that tab is tab and it's 8 chars. I am interested to improve the linter! (but not in this CL).
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@321 PS5, Line 321: spi_master_it85xx.data = data;
Removing "static globals of any kind" is not a goal in and of itself. […]
Yes, there will be more patches [coming soon] to add unit tests, and removing global state is a big thing that unblocks unit testing.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 6: Code-Review-1
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@308 PS6, Line 308: data->shm_io_base = 0; : data->ce_high = NULL; : data->ce_low = NULL; : data->it85xx_scratch_rom_reenter = 0; : Its a bit of a style thing, but no need to explicitly set these to 0 since: (a) you just calloc'ed the struct. (b) or, you followed my other suggestion and continue to use a static global for this struct, which, by definition, would be 0-initialized.
In this way, the code is simplified by removing boiler-plate, and we can focus the reader on the parts that do matter - instances where the variables are modified away from zero.
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@330 PS6, Line 330: /* These pointers are not used directly. They will be send to EC's : * register for indirect access. */ : base = 0xFFFFF000; : data->ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ : data->ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */ : This part is very similar between the two LPC_* cases and is a good candidate for a small refactor in a different CL.
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@348 PS6, Line 348: return 1; This error path leaks data. I recommend using a goto error pattern in this function to avoid leaks.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@242 PS6, Line 242: if (!flash || !flash->mst || !flash->mst->spi.data) { : msg_perr("Unable to extract data from flash context.\n"); : return SPI_GENERIC_ERROR; : } I don't know why gerrit lost this comment twice in a row.. You can delete this, none of these are NULL able upon entry into this callback.
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@308 PS6, Line 308: data->shm_io_base = 0; : data->ce_high = NULL; : data->ce_low = NULL; : data->it85xx_scratch_rom_reenter = 0; :
Its a bit of a style thing, but no need to explicitly set these to 0 since: […]
agree, option (a) is ideal imho.
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@330 PS6, Line 330: /* These pointers are not used directly. They will be send to EC's : * register for indirect access. */ : base = 0xFFFFF000; : data->ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ : data->ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */ :
This part is very similar between the two LPC_* cases and is a good candidate for a small refactor i […]
We already discussed this Daniel however it is orthogonal to this commit so not addressed here.
Hello build bot (Jenkins), Eizan Miyamoto, Daniel Kurtz, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#7).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 50 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/7
Hello build bot (Jenkins), Eizan Miyamoto, Daniel Kurtz, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#8).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 52 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/8
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@240 PS8, Line 240: (struct it85spi_data *) Is this explicit cast really needed? Typically I would no expect this for cast-from-void *.
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@308 PS8, Line 308: free(data); nit: In general, this would be a bit more robust using gotos:
int ret;
...
if (...) { ret = 1; goto free_data; }
...
if (...) { ret = 1; goto free_data; }
...
return 0;
free_data: free(data); return ret;
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@338 PS8, Line 338: free(data); on second thought ... if you already called register_shutdown() ... will it85xx_shutdown() get called now when if this function exits with "1"? If so, won't this now be a double free()?
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 8:
(2 comments)
addressing some comments
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@242 PS6, Line 242: if (!flash || !flash->mst || !flash->mst->spi.data) { : msg_perr("Unable to extract data from flash context.\n"); : return SPI_GENERIC_ERROR; : }
I don't know why gerrit lost this comment twice in a row.. […]
Done
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@308 PS6, Line 308: data->shm_io_base = 0; : data->ce_high = NULL; : data->ce_low = NULL; : data->it85xx_scratch_rom_reenter = 0; :
agree, option (a) is ideal imho.
I removed explicit initialisation of struct members, hope that my understanding of the comment is correct.
Hello build bot (Jenkins), Eizan Miyamoto, Daniel Kurtz, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#9).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 49 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/9
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 9:
(4 comments)
thank you!
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@348 PS6, Line 348: return 1;
This error path leaks data. I recommend using a goto error pattern in this function to avoid leaks.
As you mentioned in the later comment, reqister_shutdown() should take care and free data in the case. Also here is the next patch improving init flow: https://review.coreboot.org/c/flashrom/+/48196
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@240 PS8, Line 240: (struct it85spi_data *)
Is this explicit cast really needed? Typically I would no expect this for cast-from-void *.
Yes maybe the assignment makes this cast implicit, but I feel it's better to be explicit here.
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@308 PS8, Line 308: free(data);
nit: In general, this would be a bit more robust using gotos: […]
This whole init flow is improved in the other patch https://review.coreboot.org/c/flashrom/+/48196
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@338 PS8, Line 338: free(data);
on second thought ... if you already called register_shutdown() ... […]
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 9: Code-Review+2
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@240 PS8, Line 240: (struct it85spi_data *)
Yes maybe the assignment makes this cast implicit, but I feel it's better to be explicit here.
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#allocating-m...
Loosely related kernel style is to not cast from void * to specific type:
``` Casting the return value which is a void pointer is redundant. The conversion from void pointer to any other pointer type is guaranteed by the C programming language. ```
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@240 PS8, Line 240: (struct it85spi_data *)
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#allocating-m... […]
Tracking towards general kernel style is a compelling reason to go that route. Thanks Daniel for digging up that resource link.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@240 PS8, Line 240: (struct it85spi_data *)
Tracking towards general kernel style is a compelling reason to go that route. […]
Compiler is not cooperative: error: initialization discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
So I am keeping the cast
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 10: Code-Review-1
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/8/it85spi.c@240 PS8, Line 240: (struct it85spi_data *)
Compiler is not cooperative: […]
A motivation for removing extraneous casts is to only use casts in situations where they are adding semantic value - that is, where we are explicitly trying to change the pointer type in a fundamental way - such as converting from one memory layout (struct) to another, or removing const-ness. For all other cases, relying on default C language cast behavior provides simpler and more reliable code, and let's the compiler do its job easier...
In this particular case, removing the cast lets the compiler correctly expose a bug - that we are now trying to create a local non-const pointer to a struct that we have previously declared `const` - in particular this functions first parameter, which was declared `const struct flashctx *flash`.
Luckily, we don't actually intend to modify `data` via this pointer, so we needn't de-constify it. Thus, the fix here is to use:
const struct it85spi_data *data = flash->mst->spi.data;
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c@240 PS10, Line 240: struct it85spi_data *data = (struct it85spi_data *) flash->mst->spi.data; @Daniel, this is a reply to your comment - for some reasons I can't reply to it because I don't see it in the list of comments.
So I started from this: const struct it85spi_data *data = flash->mst->spi.data; and this required it85xx_enter_scratch_rom argument to become const as well. Ok, makes sense, I made the argument const - but actually, looks like it is modified in the middle of it85xx_enter_scratch_rom , specifically it85spi.c:159:35: error: increment of member 'it85xx_scratch_rom_reenter' in read-only object 159 | data->it85xx_scratch_rom_reenter++;
So data is modified, means it is not const? I realised I can't fix this quickly, I need to understand what's going on here.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c@240 PS10, Line 240: struct it85spi_data *data = (struct it85spi_data *) flash->mst->spi.data;
@Daniel, this is a reply to your comment - for some reasons I can't reply to it because I don't see […]
Take away here is... yes, `const`-ification causes one to think a bit harder about what parameters / objects should be being modified and where. This is generally a good thing, but takes more time and effort to get just right.
I think I was only actually partially incorrect in the earlier analysis. The `const struct flashctx *flash` may not be the issue, but rather the fact that `struct spi_master` itself defines a `const void *data`:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thir...
This suggests that the current intention is that spi_master's `data` fields are immutable state, and should not be modified at runtime.
This sounds like a more fundamental issue with this patch series. I suggest forking off a separate stand-alone preliminary patch to de-constify's `data` in the common code, before this patch that re-purposes it for use to point to modifiable runtime state in this spi_master driver.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 10: Code-Review-1
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c@240 PS10, Line 240: struct it85spi_data *data = (struct it85spi_data *) flash->mst->spi.data;
Take away here is... […]
I strongly oppose to the use of casts to discard const qualifiers. If the pointer is referencing data marked as const, then modifying data through this pointer will cause undefined behavior.
I agree that things as-is are pretty messy, but that's the perfect reason to not try to sort this out quickly, and instead re-think the situation thoroughly.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c@240 PS10, Line 240: struct it85spi_data *data = (struct it85spi_data *) flash->mst->spi.data;
I strongly oppose to the use of casts to discard const qualifiers. […]
Angel, nice to meet you!
I am currently trying to understand spi_master.data, and why it is const (so we are on the same page on "re-think the situation thoroughly"). Original commit where spi_master.data was introduced does not have much info on why data should be const. Maybe only the pointer can be const, but the data can be modified.
Hello build bot (Jenkins), Eizan Miyamoto, Daniel Kurtz, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#11).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 49 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/11
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c@240 PS10, Line 240: struct it85spi_data *data = (struct it85spi_data *) flash->mst->spi.data;
Angel, nice to meet you! […]
Good news: data is not const anymore (this is resolved in the earlier commit)! So I can remove explicit cast from here.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/10/it85spi.c@240 PS10, Line 240: struct it85spi_data *data = (struct it85spi_data *) flash->mst->spi.data;
Good news: data is not const anymore (this is resolved in the earlier commit)! […]
This is what I had exactly the same thing in mind as well Daniel+Angel. I've prepared that to unblock Anastasia not to be stuck too deep into the weeds of Flashrom.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c@77 PS11, Line 77: unsigned int shm_io_base; For 100% consistency with existing code, this should be:
#ifdef LPC_IO unsigned int shm_io_base; #endif
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c@77 PS11, Line 77: unsigned int shm_io_base;
For 100% consistency with existing code, this should be: […]
In existing code, wrapping this with ifdef-endif was needed, because otherwise compiler would produce a warning that shm_io_base is unused (it was not used on !LPC_IO code path). After shm_io_base becomes a member of a struct, compiler checks that struct is used (and it is), does not check individual members usage. The only reason to have ifdef-endif around shm_io_base was to avoid compiler warning, and since it is not an issue anymore - ifdef-endif not needed anymore here.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c@77 PS11, Line 77: unsigned int shm_io_base;
In existing code, wrapping this with ifdef-endif was needed, because otherwise compiler would produc […]
Similar to the `const` case, the compiler is doing its job correctly, so let's think about what it is trying to tell us:
If we remove the ifdef-endif block around the `shm_io_base` variable declaration in the original code, and build with `LPC_IO` not defined, then the compiler will rightly warn us that shm_io_base was "unused" (ie we've reserved some static memory in .bss that are never accessed, thus we could save some memory by not declaring this static global variable at all).
In other words, the compiler is telling us that we needn't define this variable at all.
In the new code, `shm_io_base` has now become a struct field. Thus, the compiler won't reserve any memory for it in `.bss` at build time. Instead, the effect is now that any structs of this type that are created at runtime will now have this little extra bit of memory allocated for this field - but the field will still be unused! The compiler, however, isn't able to detect this, and thus doesn't throw a warning ... but this is still unused memory.
It isn't a huge deal in this little instance, more of a lesson in how to do a refactoring patch like this: 1) try to keep the code consistent with the original code 2) try to avoid allocating memory that won't be used
Now, you could argue that using "ifdef/endif" like this is brittle, overly complex, and the memory savings are tiny and an over-optimization that has no practical value - and I would generally agree. But, let's clean that up in a separate patch that globally replaces this compile-time logic with equivalent run-time logic that doesn't rely on the C pre-processor.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c@77 PS11, Line 77: unsigned int shm_io_base;
Similar to the `const` case, the compiler is doing its job correctly, so let's think about what it i […]
Let's not forget that this code is currently disabled (built, but not executed), because it can result in unpredictable behavior. I tried it years ago on an HP G62 laptop with an ITE IT8502E EC, and while it was able to detect the flash chip on the EC, the laptop powered itself off after a few seconds.
I wouldn't remove the ifdef around `shm_io_base` in this patch, to avoid doing too many things in a single commit. Ideally, the preprocessor shouldn't exist, and the access mode should be configurable at runtime. This can (and should) be done in a separate commit, if there's interest.
Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/comment/de367971_914b2c77 PS11, Line 22: : #if defined(__i386__) || defined(__x86_64__) In answer to your questions regarding 'LPC_IO' && 'LPC_MEMORY'; probably we have a simple initial patch here to convert hunks with this pattern:
``` #ifdef LPC_IO .... #endif #ifdef LPC_MEMORY .... #endif ```
into,
``` #ifdef LPC_IO .... #elif LPC_MEMORY .... #endif ```
and that the defines for either should be at the top of the file with the guard `if defined(__i386__) || defined(__x86_64__)` modified to check if either is defined or just don't compile the file.
Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/comment/46ae3219_9922316a PS11, Line 59: /* IT8502 supports two access modes: : * LPC_MEMORY: through the memory window in 0xFFFFFxxx (follow mode) : * LPC_IO: through I/O port (so called indirect memory) : */ : #undef LPC_MEMORY : #define LPC_IO this would need to be moved above the initial guard, the slightly better fix is that the Makefile pass `-DLPC_MEMORY` or `-DLPC_IO` to compile the file with the correct configuration. It's all a bit awful.
Attention is currently required from: Anastasia Klimchuk. Hello build bot (Jenkins), Eizan Miyamoto, Daniel Kurtz, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/47655
to look at the new patch set (#12).
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 49 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/47655/12
Attention is currently required from: Anastasia Klimchuk. Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 12: Code-Review+1
Attention is currently required from: Daniel Kurtz, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 12:
(3 comments)
File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/comment/cbbc03b9_d53f58b0 PS11, Line 22: : #if defined(__i386__) || defined(__x86_64__)
In answer to your questions regarding 'LPC_IO' && 'LPC_MEMORY'; probably we have a simple initial pa […]
Thank you for explanations! Based on our conversation (and comments from other people here), I will do this in a separate patch.
https://review.coreboot.org/c/flashrom/+/47655/comment/4cc067d6_242fc40a PS11, Line 59: /* IT8502 supports two access modes: : * LPC_MEMORY: through the memory window in 0xFFFFFxxx (follow mode) : * LPC_IO: through I/O port (so called indirect memory) : */ : #undef LPC_MEMORY : #define LPC_IO
this would need to be moved above the initial guard, the slightly better fix is that the Makefile pa […]
Thank you! Same as above, I will do this in a separate patch.
https://review.coreboot.org/c/flashrom/+/47655/comment/2a403824_f0deb837 PS11, Line 77: unsigned int shm_io_base;
Let's not forget that this code is currently disabled (built, but not executed), because it can resu […]
I returned back ifdef-endif around shm_io_base. I agree this should be in a separate commit.
Attention is currently required from: Daniel Kurtz, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 12: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
it85spi.c: Refactor singleton states into reentrant pattern
Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
BUG=b:172876667 TEST=builds
Change-Id: I389d34d62e753c012910aa5ff24a496b24a4753c Signed-off-by: Anastasia Klimchuk aklm@chromium.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/47655 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Daniel Kurtz djkurtz@google.com Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- M it85spi.c 1 file changed, 49 insertions(+), 35 deletions(-)
Approvals: build bot (Jenkins): Verified Daniel Kurtz: Looks good to me, but someone else must approve Edward O'Callaghan: Looks good to me, approved
diff --git a/it85spi.c b/it85spi.c index 342a6e5..33e0b84 100644 --- a/it85spi.c +++ b/it85spi.c @@ -73,11 +73,13 @@ #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4) #endif /* LPC_IO */
+struct it85spi_data { #ifdef LPC_IO -static unsigned int shm_io_base; + unsigned int shm_io_base; #endif -static unsigned char *ce_high, *ce_low; -static int it85xx_scratch_rom_reenter = 0; + unsigned char *ce_high, *ce_low; + int it85xx_scratch_rom_reenter; +};
/* This function will poll the keyboard status register until either * an expected value shows up, or the timeout is reached. @@ -106,12 +108,12 @@
/* IT8502 employs a scratch RAM when flash is being updated. Call the following * two functions before/after flash erase/program. */ -static void it85xx_enter_scratch_rom(void) +static void it85xx_enter_scratch_rom(struct it85spi_data *data) { int ret, tries;
msg_pdbg("%s():%d was called ...\n", __func__, __LINE__); - if (it85xx_scratch_rom_reenter > 0) + if (data->it85xx_scratch_rom_reenter > 0) return;
#if 0 @@ -156,14 +158,14 @@
if (tries < MAX_TRY) { /* EC already runs on SRAM */ - it85xx_scratch_rom_reenter++; + data->it85xx_scratch_rom_reenter++; msg_pdbg("%s():%d * SUCCESS.\n", __func__, __LINE__); } else { msg_perr("%s():%d * Max try reached.\n", __func__, __LINE__); } }
-static void it85xx_exit_scratch_rom(void) +static void it85xx_exit_scratch_rom(struct it85spi_data *data) { #if 0 int ret; @@ -171,7 +173,7 @@ int tries;
msg_pdbg("%s():%d was called ...\n", __func__, __LINE__); - if (it85xx_scratch_rom_reenter <= 0) + if (data->it85xx_scratch_rom_reenter <= 0) return;
for (tries = 0; tries < MAX_TRY; ++tries) { @@ -199,7 +201,7 @@ }
if (tries < MAX_TRY) { - it85xx_scratch_rom_reenter = 0; + data->it85xx_scratch_rom_reenter = 0; msg_pdbg("%s():%d * SUCCESS.\n", __func__, __LINE__); } else { msg_perr("%s():%d * Max try reached.\n", __func__, __LINE__); @@ -218,7 +220,8 @@ static int it85xx_shutdown(void *data) { msg_pdbg("%s():%d\n", __func__, __LINE__); - it85xx_exit_scratch_rom(); + it85xx_exit_scratch_rom(data); + free(data);
return 0; /* FIXME: Should probably return something meaningful */ } @@ -235,49 +238,50 @@ const unsigned char *writearr, unsigned char *readarr) { - unsigned int i; + unsigned int i; + struct it85spi_data *data = flash->mst->spi.data;
- it85xx_enter_scratch_rom(); + it85xx_enter_scratch_rom(data); /* Exit scratch ROM ONLY when programmer shuts down. Otherwise, the * temporary flash state may halt the EC. */
#ifdef LPC_IO - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); - INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_low) >> 8) & 0xff); + INDIRECT_A1(data->shm_io_base, (((unsigned long int)data->ce_high) >> 8) & 0xff); + INDIRECT_WRITE(data->shm_io_base, 0xFF); /* Write anything to this address.*/ + INDIRECT_A1(data->shm_io_base, (((unsigned long int)data->ce_low) >> 8) & 0xff); #endif #ifdef LPC_MEMORY - mmio_writeb(0, ce_high); + mmio_writeb(0, data->ce_high); #endif for (i = 0; i < writecnt; ++i) { #ifdef LPC_IO - INDIRECT_WRITE(shm_io_base, writearr[i]); + INDIRECT_WRITE(data->shm_io_base, writearr[i]); #endif #ifdef LPC_MEMORY - mmio_writeb(writearr[i], ce_low); + mmio_writeb(writearr[i], data->ce_low); #endif } for (i = 0; i < readcnt; ++i) { #ifdef LPC_IO - readarr[i] = INDIRECT_READ(shm_io_base); + readarr[i] = INDIRECT_READ(data->shm_io_base); #endif #ifdef LPC_MEMORY - readarr[i] = mmio_readb(ce_low); + readarr[i] = mmio_readb(data->ce_low); #endif } #ifdef LPC_IO - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); - INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ + INDIRECT_A1(data->shm_io_base, (((unsigned long int)data->ce_high) >> 8) & 0xff); + INDIRECT_WRITE(data->shm_io_base, 0xFF); /* Write anything to this address.*/ #endif #ifdef LPC_MEMORY - mmio_writeb(0, ce_high); + mmio_writeb(0, data->ce_high); #endif
return 0; }
-static const struct spi_master spi_master_it85xx = { +static struct spi_master spi_master_it85xx = { .max_data_read = 64, .max_data_write = 64, .command = it85xx_spi_send_command, @@ -291,31 +295,41 @@ { chipaddr base;
+ struct it85spi_data *data = calloc(1, sizeof(struct it85spi_data)); + if (!data) { + msg_perr("Unable to allocate space for extra SPI master data.\n"); + return SPI_GENERIC_ERROR; + } + + spi_master_it85xx.data = data; + msg_pdbg("%s():%d superio.vendor=0x%02x\n", __func__, __LINE__, s.vendor);
- if (register_shutdown(it85xx_shutdown, NULL)) + if (register_shutdown(it85xx_shutdown, data)) { + free(data); return 1; + }
#ifdef LPC_IO /* Get LPCPNP of SHM. That's big-endian. */ sio_write(s.port, LDNSEL, 0x0F); /* Set LDN to SHM (0x0F) */ - shm_io_base = (sio_read(s.port, SHM_IO_BAR0) << 8) + + data->shm_io_base = (sio_read(s.port, SHM_IO_BAR0) << 8) + sio_read(s.port, SHM_IO_BAR1); - msg_pdbg("%s():%d shm_io_base=0x%04x\n", __func__, __LINE__, - shm_io_base); + msg_pdbg("%s():%d it85spi_data->shm_io_base=0x%04x\n", __func__, __LINE__, + data->shm_io_base);
/* These pointers are not used directly. They will be send to EC's * register for indirect access. */ base = 0xFFFFF000; - ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ - ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */ + data->ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ + data->ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */
/* pre-set indirect-access registers since in most of cases they are * 0xFFFFxx00. */ - INDIRECT_A0(shm_io_base, base & 0xFF); - INDIRECT_A2(shm_io_base, (base >> 16) & 0xFF); - INDIRECT_A3(shm_io_base, (base >> 24)); + INDIRECT_A0(data->shm_io_base, base & 0xFF); + INDIRECT_A2(data->shm_io_base, (base >> 16) & 0xFF); + INDIRECT_A3(data->shm_io_base, (base >> 24)); #endif #ifdef LPC_MEMORY /* FIXME: We should block accessing that region for anything else. @@ -327,8 +341,8 @@
msg_pdbg("%s():%d base=0x%08x\n", __func__, __LINE__, (unsigned int)base); - ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ - ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ + data->ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ + data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ #endif
return 0;
-
Anastasia Klimchuk (Code Review) -
Anastasia Klm (Code Review)
-
Angel Pons (Code Review)
-
Daniel Kurtz (Code Review)
-
Edward O'Callaghan (Code Review)
-
Eizan Miyamoto (Code Review)