Change in flashrom[master]: Simplifying the initialisation flow for it85spi
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-MessageType: newchange
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. -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Comment-Date: Tue, 01 Dec 2020 05:31:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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.
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Comment-Date: Tue, 01 Dec 2020 05:40:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: newpatchset
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?
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 08 Dec 2020 02:39:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
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. -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 08 Dec 2020 03:33:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 08 Dec 2020 04:16:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 08 Dec 2020 04:28:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 08 Dec 2020 08:29:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
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
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 15 Dec 2020 00:46:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm@chromium.org> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 15 Dec 2020 00:55:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
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. -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-Comment-Date: Tue, 15 Dec 2020 03:24:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-Comment-Date: Tue, 15 Dec 2020 03:36:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Kurtz <djkurtz@google.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-MessageType: newpatchset
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`
-- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-CC: Eizan Miyamoto <eizan@chromium.org> Gerrit-Comment-Date: Tue, 05 Jan 2021 06:29:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Kurtz <djkurtz@google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-CC: Eizan Miyamoto <eizan@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Comment-Date: Tue, 19 Jan 2021 01:22:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 8 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-CC: Eizan Miyamoto <eizan@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Anastasia Klimchuk <aklm@chromium.org> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 8 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-CC: Eizan Miyamoto <eizan@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Comment-Date: Tue, 19 Jan 2021 13:25:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/flashrom/+/48196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac1b4ae9c6f34c5265e729ae0a80024800c3c272 Gerrit-Change-Number: 48196 Gerrit-PatchSet: 9 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-CC: Eizan Miyamoto <eizan@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
participants (3)
-
Anastasia Klimchuk (Code Review) -
Daniel Kurtz (Code Review)
-
Edward O'Callaghan (Code Review)