Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
soc/amd/common: Correct SPI FIFO size check
When checking that command and data fit in the FIFO, don't count the first byte. The command doesn't go through the FIFO.
TEST=confirm error (4+68>71) goes away on Mandolin BUG=b:146225550
Change-Id: Ica2ca514deea401c9c5396913087e07a12ab3cf3 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/spi/fch_spi_flash.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/37721/1
diff --git a/src/soc/amd/common/block/spi/fch_spi_flash.c b/src/soc/amd/common/block/spi/fch_spi_flash.c index 72bc5d6..93b8a14 100644 --- a/src/soc/amd/common/block/spi/fch_spi_flash.c +++ b/src/soc/amd/common/block/spi/fch_spi_flash.c @@ -39,7 +39,8 @@ int ret; u8 buff[SPI_FIFO_DEPTH + 1];
- if ((cmd_len + data_len) > SPI_FIFO_DEPTH) + /* Ensure FIFO is large enough. First byte of command does not go in the FIFO. */ + if ((cmd_len - 1 + data_len) > SPI_FIFO_DEPTH) return -1; memcpy(buff, cmd, cmd_len); memcpy(buff + cmd_len, data, data_len);
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1: Code-Review+2
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1: Code-Review+1
LGTM. Parallels the check in fch_spi_ctrl.c
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1: Code-Review-1
The implementation in soc/amd was probably forked while some other changes in SPI subsystem were in progress. Some things were probably lost in rebase, grep for SPI_CNTRLR_DEDUCT_CMD_LEN, notice it being used for {agesa,pi}/hudson.
Ideally amd/block/spi would work with hudson too.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
The implementation in soc/amd was probably forked while some other changes in SPI subsystem were in progress. Some things were probably lost in rebase, grep for SPI_CNTRLR_DEDUCT_CMD_LEN, notice it being used for {agesa,pi}/hudson.
Ideally amd/block/spi would work with hudson too.
We would've forked mid-2017. IIRC correctly from earlier this year, Richard was asked to write a brand new driver for common/block/spi and get us away from the generic one and variable length arrays. FWIW, it looks like we used to have SPI_CNTRLR_DEDUCT_CMD_LEN | _DEDUCT_OPCODE_LEN in our old spi.c. Things look a bit different now than hudson. I agree the common code should work on all AMD FCHs, but I never had the time to review his code closely. Not sure I see where VLAs went away yet.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
Patch Set 1:
The implementation in soc/amd was probably forked while some other changes in SPI subsystem were in progress. Some things were probably lost in rebase, grep for SPI_CNTRLR_DEDUCT_CMD_LEN, notice it being used for {agesa,pi}/hudson.
Ideally amd/block/spi would work with hudson too.
We would've forked mid-2017. IIRC correctly from earlier this year, Richard was asked to write a brand new driver for common/block/spi and get us away from the generic one and variable length arrays. FWIW, it looks like we used to have SPI_CNTRLR_DEDUCT_CMD_LEN | _DEDUCT_OPCODE_LEN in our old spi.c. Things look a bit different now than hudson. I agree the common code should work on all AMD FCHs, but I never had the time to review his code closely. Not sure I see where VLAs went away yet.
I have pending work on it and will push a patch soon. AFAIK not all hudsons will be able to use the common SPI block (lack of shadow registers at SPIx4X)
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
The implementation in soc/amd was probably forked while some other changes in SPI subsystem were in progress. Some things were probably lost in rebase, grep for SPI_CNTRLR_DEDUCT_CMD_LEN, notice it being used for {agesa,pi}/hudson.
Ideally amd/block/spi would work with hudson too.
We would've forked mid-2017. IIRC correctly from earlier this year, Richard was asked to write a brand new driver for common/block/spi and get us away from the generic one and variable length arrays. FWIW, it looks like we used to have SPI_CNTRLR_DEDUCT_CMD_LEN | _DEDUCT_OPCODE_LEN in our old spi.c. Things look a bit different now than hudson. I agree the common code should work on all AMD FCHs, but I never had the time to review his code closely. Not sure I see where VLAs went away yet.
I have pending work on it and will push a patch soon. AFAIK not all hudsons will be able to use the common SPI block (lack of shadow registers at SPIx4X)
After looking deeper into the SPI block code I can see why this is a brand new driver. Especially the existence of "non_standard_sst_*" makes it not necessarily common or generic. Given the situation, I put my work on the common SPI block for AMD on hold until it clarifies a bit. For now, the only way I see it could be "common" is to separate the "new driver" exclusively used by Picasso and Stoneyridge. But maybe I am lacking the rationale why the SPI block ended like this. Any guidance on how to address it very welcome and appreicated.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1: Code-Review+1
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1: Code-Review+2
Patch Set 1: Code-Review-1 Ideally amd/block/spi would work with hudson too.
Why would soc/amd code apply to southbridge/amd chips? Isn't it ok to draw a line and say that the soc blocks will only support chips in the soc directory?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1: Code-Review-2
Patch Set 1: Code-Review+2
Patch Set 1: Code-Review-1 Ideally amd/block/spi would work with hudson too.
Why would soc/amd code apply to southbridge/amd chips? Isn't it ok to draw a line and say that the soc blocks will only support chips in the soc directory?
My -1 is because of not using the introduced to SPI subsystem that tells command byte will not consume FIFO. Giving this -2 and hoping Furquan will comment as he probably nows the reasoning best why the flag was added.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-2
Patch Set 1: Code-Review+2
Patch Set 1: Code-Review-1 Ideally amd/block/spi would work with hudson too.
Why would soc/amd code apply to southbridge/amd chips? Isn't it ok to draw a line and say that the soc blocks will only support chips in the soc directory?
My -1 is because of not using the introduced to SPI subsystem that tells command byte will not consume FIFO. Giving this -2 and hoping Furquan will comment as he probably nows the reasoning best why the flag was added.
-------/* Deduct the command length from the spi_crop_chunk() calculation for ------- sizing a transaction. */ -------SPI_CNTRLR_DEDUCT_CMD_LEN = 1 << 0, -------/* Remove the opcode size from the command length used in the ------- spi_crop_chunk() calculation. Controllers which have a dedicated ------- register for the command byte would set this flag which would ------- allow the use of the maximum transfer size. */ -------SPI_CNTRLR_DEDUCT_OPCODE_LEN = 1 << 1,
The flags are there for describing the underlying controller hardware. The spi transactions currently add length and opcode to the buffer. These flags indicate that there are other registers aside from the fifo to hold that information. The hudson code has the following:
static const struct spi_ctrlr spi_ctrlr = {
-------.xfer_vector = xfer_vectors, -------.max_xfer_size = AMD_SB_SPI_TX_LEN, -------.flags = SPI_CNTRLR_DEDUCT_CMD_LEN,
};
And it also implements xfer_vector() which is what the majority of the code takes advantage of in the code base. What I think Kyösti is looking for is the spi controller code here be more conforming with the way the rest of the code is doing things.
That said, when there's a flash_probe() callback in the spi_controller object then the spi flash API is used for probing (and should then every other spi flash command). This code seems to be taking the latter path, but the code is a bit of a hybrid it seems. e.g. fch_spi_flash_probe() is sending commands through its own internal API. I didn't go through all the paths in the driver, but it looks like this code is duplicating a lot of things that are done in the generic spi code. i.e. there seems to be a lot of duplication in this code and the generic stuff. I think the code needs a broader refactoring to fit better into the current API.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
More history.
AMD common code added here with all the duplication: https://review.coreboot.org/c/coreboot/+/35018 Removal of previous working code with existing spi library code: https://review.coreboot.org/c/coreboot/+/35019
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1: Code-Review-1
I won't monitor this change further.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
I won't monitor this change further.
This code needs a major overhaul from my investigation. One thing I see that is problematic is the write protect scheme. It needs the specific commands encoded it is blocking. However, struct spi_flash doesn't expose all those commands. That seems to be small-ish undertaking, but I think we can remove 80% of the code introduced in this patch which duplicated a lot of logic: https://review.coreboot.org/c/coreboot/+/35018
I'm filing another bug to track fixing this situation up.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-1
I won't monitor this change further.
This code needs a major overhaul from my investigation. One thing I see that is problematic is the write protect scheme. It needs the specific commands encoded it is blocking. However, struct spi_flash doesn't expose all those commands. That seems to be small-ish undertaking, but I think we can remove 80% of the code introduced in this patch which duplicated a lot of logic: https://review.coreboot.org/c/coreboot/+/35018
I'm filing another bug to track fixing this situation up.
Internal bug on our end tracking this work: https://issuetracker.google.com/146928174
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
soc/amd/common: Correct SPI FIFO size check
When checking that command and data fit in the FIFO, don't count the first byte. The command doesn't go through the FIFO.
TEST=confirm error (4+68>71) goes away on Mandolin BUG=b:146225550
Change-Id: Ica2ca514deea401c9c5396913087e07a12ab3cf3 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37721 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Eric Peers epeers@google.com Reviewed-by: Frans Hendriks fhendriks@eltan.com Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/common/block/spi/fch_spi_flash.c 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Frans Hendriks: Looks good to me, but someone else must approve Raul Rangel: Looks good to me, approved Eric Peers: Looks good to me, but someone else must approve
Objections: Kyösti Mälkki: I would prefer that you didn't submit this
diff --git a/src/soc/amd/common/block/spi/fch_spi_flash.c b/src/soc/amd/common/block/spi/fch_spi_flash.c index d8eeefc..b05c1a4 100644 --- a/src/soc/amd/common/block/spi/fch_spi_flash.c +++ b/src/soc/amd/common/block/spi/fch_spi_flash.c @@ -40,7 +40,8 @@ int ret; u8 buff[SPI_FIFO_DEPTH + 1];
- if ((cmd_len + data_len) > SPI_FIFO_DEPTH) + /* Ensure FIFO is large enough. First byte of command does not go in the FIFO. */ + if ((cmd_len - 1 + data_len) > SPI_FIFO_DEPTH) return -1; memcpy(buff, cmd, cmd_len); memcpy(buff + cmd_len, data, data_len);
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37721 )
Change subject: soc/amd/common: Correct SPI FIFO size check ......................................................................
Patch Set 2:
(1 comment)
Pushed a strawman here: https://review.coreboot.org/c/coreboot/+/37957
https://review.coreboot.org/c/coreboot/+/37721/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/37721/2/src/soc/amd/common/block/sp... PS2, Line 44: if ((cmd_len - 1 + data_len) > SPI_FIFO_DEPTH) FWIW, this check shouldn't be in here since crop_chunk already took this into account.
-
Aaron Durbin (Code Review) -
Eric Peers (Code Review)
-
Frans Hendriks (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Michał Żygowski (Code Review)
-
Raul Rangel (Code Review)