Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Simplifying the initialisation flow for it85spi
1) Inlining it85xx_spi_common_init since it's only used once in it85xx_spi_init, after inlining ret value is not needed.
2) Creating it86_init_error section to ensure that data is freed if error happened in initialisaton flow.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 32 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/1
diff --git a/it85spi.c b/it85spi.c index 6215515..52b09b1 100644 --- a/it85spi.c +++ b/it85spi.c @@ -289,11 +289,18 @@ .write_aai = default_spi_write_aai, };
-static int it85xx_spi_common_init(struct superio s) +int it85xx_spi_init(struct superio s) { chipaddr base; + struct it85spi_data *data; + msg_pdbg("%s():%d superio.vendor=0x%02x\n", __func__, __LINE__, s.vendor);
- struct it85spi_data *data = calloc(1, sizeof(struct it85spi_data)); + if (!(internal_buses_supported & BUS_FWH)) { + msg_pdbg("%s():%d buses not support FWH\n", __func__, __LINE__); + return 1; + } + + 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; @@ -301,12 +308,8 @@
spi_master_it85xx.data = data;
- msg_pdbg("%s():%d superio.vendor=0x%02x\n", __func__, __LINE__, - s.vendor); - if (register_shutdown(it85xx_shutdown, data)) { - free(data); - return 1; + goto it85spi_init_error; }
#ifdef LPC_IO @@ -334,9 +337,9 @@ * Major TODO here, and it will be a lot of work. */ base = (chipaddr)physmap("it85 communication", 0xFFFFF000, 0x1000); - if (base == (chipaddr)ERROR_PTR) - free(data); - return 1; + if (base == (chipaddr)ERROR_PTR) { + goto it85spi_init_error; + }
msg_pdbg("%s():%d base=0x%08x\n", __func__, __LINE__, (unsigned int)base); @@ -344,37 +347,27 @@ data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ #endif
+ msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, + internal_buses_supported); + /* Check for FWH because IT85 listens to FWH cycles. + * FIXME: The big question is whether FWH cycles are necessary + * for communication even if LPC_IO is defined. + */ + if (internal_buses_supported & BUS_FWH) + msg_pdbg("Registering IT85 SPI.\n"); + /* FIXME: Really leave FWH enabled? We can't use this region + * anymore since accessing it would mess up IT85 communication. + * If we decide to disable FWH for this region, we should print + * a debug message about it. + */ + /* Set this as SPI controller. */ + register_spi_master(&spi_master_it85xx); + return 0; -}
-int it85xx_spi_init(struct superio s) -{ - int ret; - - if (!(internal_buses_supported & BUS_FWH)) { - msg_pdbg("%s():%d buses not support FWH\n", __func__, __LINE__); - return 1; - } - ret = it85xx_spi_common_init(s); - msg_pdbg("FWH: %s():%d ret=%d\n", __func__, __LINE__, ret); - if (!ret) { - msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, - internal_buses_supported); - /* Check for FWH because IT85 listens to FWH cycles. - * FIXME: The big question is whether FWH cycles are necessary - * for communication even if LPC_IO is defined. - */ - if (internal_buses_supported & BUS_FWH) - msg_pdbg("Registering IT85 SPI.\n"); - /* FIXME: Really leave FWH enabled? We can't use this region - * anymore since accessing it would mess up IT85 communication. - * If we decide to disable FWH for this region, we should print - * a debug message about it. - */ - /* Set this as SPI controller. */ - register_spi_master(&spi_master_it85xx); - } - return ret; +it85spi_init_error: + free(data); + return 1; }
#endif
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@303 PS1, Line 303: 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; : : if (register_shutdown(it85xx_shutdown, data)) { : goto it85spi_init_error; this whole block moves towards the end of the function and therefore there is no need for goto's.
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@339 PS1, Line 339: chipaddr these casts are not needed
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@341 PS1, Line 341: goto it85spi_init_error; this should be a basic early return the code was correct before.
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@346 PS1, Line 346: data->ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ : data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ this is common code on both sides of the pre-processor directive.
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@349 PS1, Line 349: : msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, : internal_buses_supported); this isn't needed.
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@362 PS1, Line 362: */ this comment block is all related the to the above code and should be immediately close to the register_spi_master() function.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@303 PS1, Line 303: 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; : : if (register_shutdown(it85xx_shutdown, data)) { : goto it85spi_init_error;
this whole block moves towards the end of the function and therefore there is no need for goto's.
in fact don't even bother checking the return value of register_shutdown() as it's useless to do so as every call is always valid. therefore this constructor shall have no free()'s in its error paths and therefore no need for goto's as it is a single phase construction.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48196
to look at the new patch set (#2).
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Simplifying the initialisation flow for it85spi
1) Inlining it85xx_spi_common_init since it's only used once in it85xx_spi_init, after inlining ret value is not needed.
2) Moving data allocation closer to the end of init function to avoid freeing data in case of early return on error.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 50 insertions(+), 53 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/2
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@303 PS1, Line 303: 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; : : if (register_shutdown(it85xx_shutdown, data)) { : goto it85spi_init_error;
in fact don't even bother checking the return value of register_shutdown() as it's useless to do so […]
I am keeping this check for return value of register_shutdown because as it is currently implemented seems like it can return 1 or 0. If this aspect needs to be changed/improved, I can do it later as a next step. Moving everything to the bottom of the function as you suggested.
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@339 PS1, Line 339: chipaddr
these casts are not needed
Done
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@341 PS1, Line 341: goto it85spi_init_error;
this should be a basic early return the code was correct before.
Done
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@346 PS1, Line 346: data->ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ : data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */
this is common code on both sides of the pre-processor directive.
Yes I agree! Can I take care of this code behind the flags in the next patch?
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@349 PS1, Line 349: : msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, : internal_buses_supported);
this isn't needed.
There are lots of similar debug messages in this file. I am not sure what exactly they are for so I don't want to remove them. In any case, if we decide to remove them can I do it in the next patch?
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@362 PS1, Line 362: */
this comment block is all related the to the above code and should be immediately close to the regis […]
Not sure what do you mean here: do you mean this FIXME comment should be below /* Set this as SPI controller */ one? So just swap comments?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@317 PS2, Line 317: ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ : ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */ this is common code, move it to after the ifdef blocks.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@331 PS2, Line 331: { no need for '{ .. }' for single statements under a branch.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@341 PS2, Line 341: msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, : internal_buses_supported); : /* Check for FWH because IT85 listens to FWH cycles. : * FIXME: The big question is whether FWH cycles are necessary : * for communication even if LPC_IO is defined. : */ : if (internal_buses_supported & BUS_FWH) : msg_pdbg("Registering IT85 SPI.\n"); this whole block constitutes checks and prints of inputs parameters to this function and therefore should be at the beginning of the function - probably after the `if (!(internal_buses_supported & BUS_FWH)) {` block above.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@354 PS2, Line 354: if (!data) { You are checking for nullity _after_ you deference here. Remember the pattern 'check-then-do'.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@360 PS2, Line 360: free(data); It maybe a bit more clear now for you that this just papers over the issue that register_shutdown() should never fail. Notice now that physmap() leaks and that the shutdown callback is responsible for unmapping it. Putting a free() here (not that it matters) is just distracting away from the real leaks that do matter.
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48196
to look at the new patch set (#3).
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Simplifying the initialisation flow for it85spi
1) Inlining it85xx_spi_common_init since it's only used once in it85xx_spi_init, after inlining ret value is not needed.
2) Moving data allocation closer to the end of init function to avoid freeing data in case of early return on error.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 48 insertions(+), 53 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/3
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@317 PS2, Line 317: ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ : ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */
this is common code, move it to after the ifdef blocks.
Done
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@331 PS2, Line 331: {
no need for '{ .. }' for single statements under a branch.
Done
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@341 PS2, Line 341: msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, : internal_buses_supported); : /* Check for FWH because IT85 listens to FWH cycles. : * FIXME: The big question is whether FWH cycles are necessary : * for communication even if LPC_IO is defined. : */ : if (internal_buses_supported & BUS_FWH) : msg_pdbg("Registering IT85 SPI.\n");
this whole block constitutes checks and prints of inputs parameters to this function and therefore s […]
I moved the first debug message above, but the last makes more sense to move below, just above register_spi_master. if (internal_buses_supported & BUS_FWH) becomes redundant because if this is false than function returns early and prints error message.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@354 PS2, Line 354: if (!data) {
You are checking for nullity _after_ you deference here. Remember the pattern 'check-then-do'.
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c@310 PS3, Line 310: } ``` } else msg_pdbg("Registering IT85 SPI.\n"); ```
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c@342 PS3, Line 342: ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ : ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ looks like you can just fold these into the below initialisation. ``` data->ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ ```
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c@362 PS3, Line 362: msg_pdbg("Registering IT85 SPI.\n"); remove
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48196
to look at the new patch set (#4).
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Simplifying the initialisation flow for it85spi
1) Inlining it85xx_spi_common_init since it's only used once in it85xx_spi_init, after inlining ret value is not needed.
2) Moving data allocation closer to the end of init function to avoid freeing data in case of early return on error.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 58 insertions(+), 68 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/4
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c@310 PS3, Line 310: }
Is else needed here ? if condition returns, so without else it is the same effect. I added else, because maybe it is needed for readability purposes? Tell me if I need to remove.
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c@342 PS3, Line 342: ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ : ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */
looks like you can just fold these into the below initialisation. […]
wow yes ! this is looking so nicer
https://review.coreboot.org/c/flashrom/+/48196/3/it85spi.c@362 PS3, Line 362: msg_pdbg("Registering IT85 SPI.\n");
remove
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 4: Code-Review+2
(4 comments)
https://review.coreboot.org/c/flashrom/+/48196/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/48196/4//COMMIT_MSG@7 PS4, Line 7: Simplifying the initialisation flow for it85spi `it85spi.c: Inline it85xx_spi_common_init()`
https://review.coreboot.org/c/flashrom/+/48196/4//COMMIT_MSG@9 PS4, Line 9: 1) Inlining it85xx_spi_common_init since it's only used once in : it85xx_spi_init, after inlining ret value is not needed. no need for the points.
``` Inline it85xx_spi_common_init() to single call site of `it85xx_spi_init() as the construction is a single phase one. This allows for less cyclomatic complexity by validating early and initialisation at the eulogy of the one entry-point to the driver. ```
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@362 PS1, Line 362: */
Not sure what do you mean here: do you mean this FIXME comment should be below /* Set this as SPI co […]
Ack
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@360 PS2, Line 360: free(data);
It maybe a bit more clear now for you that this just papers over the issue that register_shutdown() […]
Ack
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48196
to look at the new patch set (#5).
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
it85spi.c: Inline it85xx_spi_common_init()
Inline it85xx_spi_common_init() to single call site of `it85xx_spi_init() as the construction is a single phase one. This allows for less cyclomatic complexity by validating early and initialisation at the eulogy of the one entry-point to the driver.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 58 insertions(+), 68 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/5
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48196
to look at the new patch set (#6).
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
it85spi.c: Inline it85xx_spi_common_init()
Inline it85xx_spi_common_init() to single call site of it85xx_spi_init() as the construction is a single phase one. This allows for less cyclomatic complexity by validating early and initialisation at the eulogy of the one entry-point to the driver.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 58 insertions(+), 68 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/6
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
Patch Set 6:
(2 comments)
thank you!
https://review.coreboot.org/c/flashrom/+/48196/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/48196/4//COMMIT_MSG@7 PS4, Line 7: Simplifying the initialisation flow for it85spi
`it85spi. […]
Done
https://review.coreboot.org/c/flashrom/+/48196/4//COMMIT_MSG@9 PS4, Line 9: 1) Inlining it85xx_spi_common_init since it's only used once in : it85xx_spi_init, after inlining ret value is not needed.
no need for the points. […]
Done
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@299 PS6, Line 299: %s Be consistent with %s(). Probably don't need __LINE__. Better to just combine both of these into a single debug string.
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@309 PS6, Line 309: else else not needed.
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@348 PS6, Line 348: data->shm_io_base = shm_io_base; Does compiler complain that `shm_io_base` is used while undefined if `!LPC_IO`?
Perhaps move this assignment into the `#if LPC_IO` block if it is only relevant in that case.
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@350 PS6, Line 350: base + 0xD00 The value here depends on exact type of `chipaddr`. Is chipaddr a pointer type or an integer type?
The old 'LPC_IO' code path `((unsigned char *)base) + 0xE00;` was a bit more explicit about what exactly was happening by first casting base to `unsigned char *`. In this way, the addition is always counting bytes and not accidentally counting 32-bit words or some other pointer size.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@348 PS6, Line 348: data->shm_io_base = shm_io_base;
Does compiler complain that `shm_io_base` is used while undefined if `!LPC_IO`? […]
Probably reasonable to just assign it to zero at its declaration above Anastasia, `unsigned int shm_io_base = 0;`
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@350 PS6, Line 350: base + 0xD00
The value here depends on exact type of `chipaddr`. Is chipaddr a pointer type or an integer type? […]
+1 to Daniel's comment here, my eye didn't even notice that :O
Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48196
to look at the new patch set (#7).
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
it85spi.c: Inline it85xx_spi_common_init()
Inline it85xx_spi_common_init() to single call site of it85xx_spi_init() as the construction is a single phase one. This allows for less cyclomatic complexity by validating early and initialisation at the eulogy of the one entry-point to the driver.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 58 insertions(+), 68 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/7
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@299 PS6, Line 299: %s
Be consistent with %s(). Probably don't need __LINE__. […]
Combined into a single string. I would keep __LINE__ maybe it was useful for someone who added it?
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@309 PS6, Line 309: else
else not needed.
Done
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@348 PS6, Line 348: data->shm_io_base = shm_io_base;
Probably reasonable to just assign it to zero at its declaration above Anastasia, `unsigned int shm_ […]
Compiler did not complain, but I wouldn't mind it complaining in this case. shm_io_base is now a member of the struct, and all members exist regardless of LPC_IO, so I want to assign it anyway even if !LPC_IO (so doing what Ed suggested).
https://review.coreboot.org/c/flashrom/+/48196/6/it85spi.c@350 PS6, Line 350: base + 0xD00
+1 to Daniel's comment here, my eye didn't even notice that :O
chipaddr is both pointer and integer... uintptr_t This was written differently between LPC_IO and LPC_MEMORY code, but worked the same because of uintptr_t. Agree to change to `((unsigned char *)base) + 0xE00`
Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
Patch Set 7: Code-Review+2
Attention is currently required from: Anastasia Klimchuk. Hello build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/48196
to look at the new patch set (#8).
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
it85spi.c: Inline it85xx_spi_common_init()
Inline it85xx_spi_common_init() to single call site of it85xx_spi_init() as the construction is a single phase one. This allows for less cyclomatic complexity by validating early and initialisation at the eulogy of the one entry-point to the driver.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org --- M it85spi.c 1 file changed, 60 insertions(+), 67 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/48196/8
Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
Patch Set 8: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: it85spi.c: Inline it85xx_spi_common_init() ......................................................................
it85spi.c: Inline it85xx_spi_common_init()
Inline it85xx_spi_common_init() to single call site of it85xx_spi_init() as the construction is a single phase one. This allows for less cyclomatic complexity by validating early and initialisation at the eulogy of the one entry-point to the driver.
BUG=b:172876667 TEST=builds
Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Signed-off-by: Anastasia Klimchuk aklm@chromium.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/48196 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- M it85spi.c 1 file changed, 60 insertions(+), 67 deletions(-)
Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved
diff --git a/it85spi.c b/it85spi.c index 33e0b84..9817c2a 100644 --- a/it85spi.c +++ b/it85spi.c @@ -291,91 +291,84 @@ .write_aai = default_spi_write_aai, };
-static int it85xx_spi_common_init(struct superio s) +int it85xx_spi_init(struct superio s) { chipaddr base; + struct it85spi_data *data; + unsigned int shm_io_base = 0;
- struct it85spi_data *data = calloc(1, sizeof(struct it85spi_data)); + msg_pdbg("%s():%d superio.vendor=0x%02x internal_buses_supported=0x%x\n", + __func__, __LINE__, s.vendor, internal_buses_supported); + + /* Check for FWH because IT85 listens to FWH cycles. + * FIXME: The big question is whether FWH cycles are necessary + * for communication even if LPC_IO is defined. + */ + if (!(internal_buses_supported & BUS_FWH)) { + msg_pdbg("%s():%d buses not support FWH\n", __func__, __LINE__); + return 1; + } + + msg_pdbg("Registering IT85 SPI.\n"); + +#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) + + sio_read(s.port, SHM_IO_BAR1); + msg_pdbg("%s():%d shm_io_base=0x%04x\n", __func__, __LINE__, + shm_io_base); + + /* These pointers are not used directly. They will be send to EC's + * register for indirect access. */ + base = 0xFFFFF000; + + /* 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)); +#endif +#ifdef LPC_MEMORY + /* FIXME: We should block accessing that region for anything else. + * Major TODO here, and it will be a lot of work. + */ + base = physmap("it85 communication", 0xFFFFF000, 0x1000); + if (base == ERROR_PTR) + return 1; + + msg_pdbg("%s():%d base=0x%08x\n", __func__, __LINE__, + (unsigned int)base); +#endif + + 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); +#ifdef LPC_IO + data->shm_io_base = shm_io_base; +#endif + data->ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ + data->ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */
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) */ - data->shm_io_base = (sio_read(s.port, SHM_IO_BAR0) << 8) + - sio_read(s.port, SHM_IO_BAR1); - msg_pdbg("%s():%d it85spi_data->shm_io_base=0x%04x\n", __func__, __LINE__, - data->shm_io_base); + spi_master_it85xx.data = data;
- /* 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 */ - - /* pre-set indirect-access registers since in most of cases they are - * 0xFFFFxx00. */ - 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. - * Major TODO here, and it will be a lot of work. + /* FIXME: Really leave FWH enabled? We can't use this region + * anymore since accessing it would mess up IT85 communication. + * If we decide to disable FWH for this region, we should print + * a debug message about it. */ - base = (chipaddr)physmap("it85 communication", 0xFFFFF000, 0x1000); - if (base == (chipaddr)ERROR_PTR) - return 1; - - msg_pdbg("%s():%d base=0x%08x\n", __func__, __LINE__, - (unsigned int)base); - data->ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ - data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */ -#endif + /* Set this as SPI controller. */ + register_spi_master(&spi_master_it85xx);
return 0; }
-int it85xx_spi_init(struct superio s) -{ - int ret; - - if (!(internal_buses_supported & BUS_FWH)) { - msg_pdbg("%s():%d buses not support FWH\n", __func__, __LINE__); - return 1; - } - ret = it85xx_spi_common_init(s); - msg_pdbg("FWH: %s():%d ret=%d\n", __func__, __LINE__, ret); - if (!ret) { - msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, - internal_buses_supported); - /* Check for FWH because IT85 listens to FWH cycles. - * FIXME: The big question is whether FWH cycles are necessary - * for communication even if LPC_IO is defined. - */ - if (internal_buses_supported & BUS_FWH) - msg_pdbg("Registering IT85 SPI.\n"); - /* FIXME: Really leave FWH enabled? We can't use this region - * anymore since accessing it would mess up IT85 communication. - * If we decide to disable FWH for this region, we should print - * a debug message about it. - */ - /* Set this as SPI controller. */ - register_spi_master(&spi_master_it85xx); - } - return ret; -} - #endif
-
Anastasia Klimchuk (Code Review) -
Daniel Kurtz (Code Review)
-
Edward O'Callaghan (Code Review)