Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
sb600spi: Add spireadmode
Added spireadmode for >= Bolton. Do not override speed or read mode for >= Bolton if parameter not specified. Minor cleanup of sb600spi.c code.
TEST=Manual: deploy on tremblye read flash using various parameters BUG=b:147665085,b:147666328 BRANCH=master
Change-Id: Id7fec7eb87ff811148217dc56a86dca3fef122ff Signed-off-by: Rob Barnes robbarnes@google.com --- M flashrom.8.tmpl M sb600spi.c 2 files changed, 90 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/38833/1
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index aa5bcd4..fde98c0 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -497,11 +497,29 @@ .sp * SB6xx, SB7xx, SP5xxx: from 16.5 MHz up to and including 33 MHz .sp +-The default is to use 16.5 MHz and disable Fast Reads. +.sp * SB8xx, SB9xx, Hudson: from 16.5 MHz up to and including 66 MHz .sp +-The default is to use 16.5 MHz and disable Fast Reads. +.sp * Yangtze (with SPI 100 engine as found in Kabini and Tamesh): all of them .sp -The default is to use 16.5 MHz and disable Fast Reads. +-The default is to use the frequency that is currently configured. +.sp +An optional +.B spireadmode +parameter specifies the read mode of the SPI bus where applicable (Bolton or later). +Syntax is +.sp +.B " flashrom -p internal:spireadmode=mode" +.sp +where +.B mode +can be +.BR "'Normal\ (up\ to\ 33 MHz)'" ", " "'Normal\ (up\ to\ 66 MHz)'" ", " "'Dual\ IO\ (1-1-2)'" ", " "'Quad\ IO\ (1-1-4)'" ", " "'Dual\ IO\ (1-2-2)'" ", " "'Quad\ IO\ (1-4-4)'" ", or " "'Fast\ Read'" "." +.sp +The default is to use the read mode that is currently configured. .TP .B Intel chipsets .sp diff --git a/sb600spi.c b/sb600spi.c index cb5c4a4..49fe37f 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -347,23 +347,33 @@ const uint8_t speed; };
-static const struct spispeed spispeeds[] = { - { "66 MHz", 0x00 }, - { "33 MHz", 0x01 }, - { "22 MHz", 0x02 }, - { "16.5 MHz", 0x03 }, - { "100 MHz", 0x04 }, - { "Reserved", 0x05 }, - { "Reserved", 0x06 }, - { "800 kHz", 0x07 }, +static const char* spispeeds[] = { + "66 MHz", + "33 MHz", + "22 MHz", + "16.5 MHz", + "100 MHz", + "Reserved", + "Reserved", + "800 kHz", };
-static int set_speed(struct pci_dev *dev, const struct spispeed *spispeed) +static const char* spireadmodes[] = { + "Normal (up to 33 MHz)", + "Reserved", + "Dual IO (1-1-2)", + "Quad IO (1-1-4)", + "Dual IO (1-2-2)", + "Quad IO (1-4-4)", + "Normal (up to 66 MHz)", + "Fast Read", +}; + +static int set_speed(struct pci_dev *dev, uint8_t speed) { bool success = false; - uint8_t speed = spispeed->speed;
- msg_pdbg("Setting SPI clock to %s (0x%x).\n", spispeed->name, speed); + msg_pdbg("Setting SPI clock to %s (%i).\n", spispeeds[speed], speed); if (amd_gen >= CHIPSET_YANGTZE) { rmmio_writew((speed << 12) | (speed << 8) | (speed << 4) | speed, sb600_spibar + 0x22); uint16_t tmp = mmio_readw(sb600_spibar + 0x22); @@ -375,33 +385,40 @@ }
if (!success) { - msg_perr("Setting SPI clock failed.\n"); + msg_perr("Setting SPI clock to %s failed.\n", spispeeds[speed]); return 1; } + msg_pdbg("Setting SPI clock to %s succeeded.\n", spispeeds[speed]); return 0; }
-static int set_mode(struct pci_dev *dev, uint8_t read_mode) +static int set_mode(struct pci_dev *dev, uint8_t mode) { + msg_pdbg("Setting SPI read mode to %s (%i).\n", spireadmodes[mode], mode); uint32_t tmp = mmio_readl(sb600_spibar + 0x00); tmp &= ~(0x6 << 28 | 0x1 << 18); /* Clear mode bits */ - tmp |= ((read_mode & 0x6) << 28) | ((read_mode & 0x1) << 18); + tmp |= ((mode & 0x6) << 28) | ((mode & 0x1) << 18); rmmio_writel(tmp, sb600_spibar + 0x00); - if (tmp != mmio_readl(sb600_spibar + 0x00)) + if (tmp != mmio_readl(sb600_spibar + 0x00)) { + msg_perr("Setting SPI read mode to %s failed.\n", spireadmodes[mode]); return 1; + } + msg_pdbg("Setting SPI read mode to %s succeeded.\n", spireadmodes[mode]); return 0; }
static int handle_speed(struct pci_dev *dev) { uint32_t tmp; - uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */ - + int16_t spispeed_idx = -1; + int16_t spireadmode_idx = -1; char *spispeed = extract_programmer_param("spispeed"); + char *spireadmode = extract_programmer_param("spireadmode"); + if (spispeed != NULL) { unsigned int i; for (i = 0; i < ARRAY_SIZE(spispeeds); i++) { - if (strcasecmp(spispeeds[i].name, spispeed) == 0) { + if (strcasecmp(spispeeds[i], spispeed) == 0) { spispeed_idx = i; break; } @@ -421,32 +438,37 @@ free(spispeed); }
+ if (spireadmode != NULL) { + unsigned int i; + for (i = 0; i < ARRAY_SIZE(spireadmodes); i++) { + if (strcasecmp(spireadmodes[i], spireadmode) == 0) { + spireadmode_idx = i; + break; + } + } + if ((strcasecmp(spireadmode, "reserved") == 0) || + (i == ARRAY_SIZE(spireadmodes)) || + (amd_gen < CHIPSET_BOLTON)) { + msg_perr("Error: Invalid spireadmode value: '%s'.\n", spireadmode); + free(spireadmode); + return 1; + } + free(spireadmode); + } + /* See the chipset support matrix for SPI Base_Addr below for an explanation of the symbols used. * bit 6xx 7xx/SP5100 8xx 9xx hudson1 hudson234 bolton/yangtze * 18 rsvd <- fastReadEnable ? <- ? SpiReadMode[0] * 29:30 rsvd <- <- ? <- ? SpiReadMode[2:1] */ if (amd_gen >= CHIPSET_BOLTON) { - static const char *spireadmodes[] = { - "Normal (up to 33 MHz)", /* 0 */ - "Reserved", /* 1 */ - "Dual IO (1-1-2)", /* 2 */ - "Quad IO (1-1-4)", /* 3 */ - "Dual IO (1-2-2)", /* 4 */ - "Quad IO (1-4-4)", /* 5 */ - "Normal (up to 66 MHz)", /* 6 */ - "Fast Read", /* 7 (Not defined in the Bolton datasheet.) */ - }; + tmp = mmio_readl(sb600_spibar + 0x00); uint8_t read_mode = ((tmp >> 28) & 0x6) | ((tmp >> 18) & 0x1); - msg_pdbg("SpiReadMode=%s (%i)\n", spireadmodes[read_mode], read_mode); - if (read_mode != 6) { - read_mode = 6; /* Default to "Normal (up to 66 MHz)" */ - if (set_mode(dev, read_mode) != 0) { - msg_perr("Setting read mode to "%s" failed.\n", spireadmodes[read_mode]); - return 1; - } - msg_pdbg("Setting read mode to "%s" succeeded.\n", spireadmodes[read_mode]); + msg_pdbg("SPI read mode is %s (%i)\n", + spireadmodes[read_mode], read_mode); + if (spireadmode_idx >= 0 && set_mode(dev, spireadmode_idx) != 0) { + return 1; }
if (amd_gen >= CHIPSET_YANGTZE) { @@ -463,10 +485,10 @@ }
tmp = mmio_readw(sb600_spibar + 0x22); /* SPI 100 Speed Config */ - msg_pdbg("NormSpeedNew is %s\n", spispeeds[(tmp >> 12) & 0xf].name); - msg_pdbg("FastSpeedNew is %s\n", spispeeds[(tmp >> 8) & 0xf].name); - msg_pdbg("AltSpeedNew is %s\n", spispeeds[(tmp >> 4) & 0xf].name); - msg_pdbg("TpmSpeedNew is %s\n", spispeeds[(tmp >> 0) & 0xf].name); + msg_pdbg("NormSpeedNew is %s\n", spispeeds[(tmp >> 12) & 0xf]); + msg_pdbg("FastSpeedNew is %s\n", spispeeds[(tmp >> 8) & 0xf]); + msg_pdbg("AltSpeedNew is %s\n", spispeeds[(tmp >> 4) & 0xf]); + msg_pdbg("TpmSpeedNew is %s\n", spispeeds[(tmp >> 0) & 0xf]); } } else { if (amd_gen >= CHIPSET_SB89XX && amd_gen <= CHIPSET_HUDSON234) { @@ -479,9 +501,15 @@ } } tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3; - msg_pdbg("NormSpeed is %s\n", spispeeds[tmp].name); + msg_pdbg("NormSpeed is %s\n", spispeeds[tmp]); + if (spispeed_idx < 0) { + spispeed_idx = 3; /* Default to 16.5 MHz */ + } } - return set_speed(dev, &spispeeds[spispeed_idx]); + if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { + return 1; + } + return 0; }
static int handle_imc(struct pci_dev *dev)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@376 PS1, Line 376: \n Maybe replace this newline with `...` and a space, and then ... (see next comment)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@388 PS1, Line 388: Setting SPI clock to %s ... this could be shortened to "failed.\n", and ...
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@391 PS1, Line 391: Setting SPI clock to %s ... this as well.
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@397 PS1, Line 397: msg_pdbg("Setting SPI read mode to %s (%i).\n", spireadmodes[mode], mode); Same as before.
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@435 PS1, Line 435: free(spispeed); : return 1; nit: This path does not free(spireadmode), but it might be null.
Instead, one can keep the declarations at the beginning and only extract spireadmode after this block.
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@451 PS1, Line 451: (amd_gen < CHIPSET_BOLTON)) Does this mean that generations before Bolton do not support configuring spireadmode at all?
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@509 PS1, Line 509: if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { : return 1; : } : return 0; This is a bit hard to parse. How about:
if (spispeed_idx < 0) return 1;
return set_speed(dev, spispeed_idx);
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@376 PS1, Line 376: \n
Maybe replace this newline with `...` and a space, and then ... […]
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@388 PS1, Line 388: Setting SPI clock to %s
... this could be shortened to "failed.\n", and ...
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@391 PS1, Line 391: Setting SPI clock to %s
... this as well.
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@397 PS1, Line 397: msg_pdbg("Setting SPI read mode to %s (%i).\n", spireadmodes[mode], mode);
Same as before.
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@435 PS1, Line 435: free(spispeed); : return 1;
nit: This path does not free(spireadmode), but it might be null. […]
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@451 PS1, Line 451: (amd_gen < CHIPSET_BOLTON))
Does this mean that generations before Bolton do not support configuring spireadmode at all?
I'm not sure. The code already here only set spi mode for >= Bolton which is why I restricted this option to >= Bolton.
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@509 PS1, Line 509: if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { : return 1; : } : return 0;
This is a bit hard to parse. How about: […]
Agree. Except it would return 0 (success) if spispeed_idx < 0.
Hello Edward O'Callaghan, Angel Pons, David Hendricks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38833
to look at the new patch set (#2).
Change subject: sb600spi: Add spireadmode ......................................................................
sb600spi: Add spireadmode
Added spireadmode for >= Bolton. Do not override speed or read mode for >= Bolton if parameter not specified. Minor cleanup of sb600spi.c code.
TEST=Manual: deploy on tremblye read flash using various parameters BUG=b:147665085,b:147666328 BRANCH=master
Change-Id: Id7fec7eb87ff811148217dc56a86dca3fef122ff Signed-off-by: Rob Barnes robbarnes@google.com --- M flashrom.8.tmpl M sb600spi.c 2 files changed, 92 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/38833/2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@451 PS1, Line 451: (amd_gen < CHIPSET_BOLTON))
I'm not sure. […]
Looking at the code below again, it seems that spireadmode does not have an effect for chipsets < Bolton. Instead of returning, I would just ignore the parameter (and maybe print a warning that it's not being used)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@509 PS1, Line 509: if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { : return 1; : } : return 0;
Agree. Except it would return 0 (success) if spispeed_idx < 0.
Well, if spispeed_idx is negative, the previous code would have barfed. Is it a problem if set_speed is not called?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@509 PS1, Line 509: if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { : return 1; : } : return 0;
Well, if spispeed_idx is negative, the previous code would have barfed. […]
Well, the previous code defaulted to 3, so it never barfed. Still, I would consider it an error. I would at least print a warning if that happens.
Hello Edward O'Callaghan, Angel Pons, David Hendricks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38833
to look at the new patch set (#3).
Change subject: sb600spi: Add spireadmode ......................................................................
sb600spi: Add spireadmode
Added spireadmode for >= Bolton. Do not override speed or read mode for >= Bolton if parameter not specified. Minor cleanup of sb600spi.c code.
TEST=Manual: deploy on tremblye read flash using various parameters BUG=b:147665085,b:147666328 BRANCH=master
Change-Id: Id7fec7eb87ff811148217dc56a86dca3fef122ff Signed-off-by: Rob Barnes robbarnes@google.com --- M flashrom.8.tmpl M sb600spi.c 2 files changed, 100 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/38833/3
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@451 PS1, Line 451: (amd_gen < CHIPSET_BOLTON))
Looking at the code below again, it seems that spireadmode does not have an effect for chipsets < Bo […]
Done
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@509 PS1, Line 509: if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { : return 1; : } : return 0;
Well, the previous code defaulted to 3, so it never barfed. Still, I would consider it an error. […]
set_speed only writes to the register for setting the speed. Skipping this is intentional. The assumption is that the firmware should have already configured the optimal spi speed, so just default to leaving it unchanged for >= BOLTON. I added a warning for both spireadmode and spispeed when they are left unset.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 3:
Gentle Bump.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 3: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
Juuust one last thing.
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@509 PS1, Line 509: if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { : return 1; : } : return 0;
set_speed only writes to the register for setting the speed. Skipping this is intentional. […]
Ack
https://review.coreboot.org/c/flashrom/+/38833/3/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/3/sb600spi.c@453 PS3, Line 453: (amd_gen < CHIPSET_BOLTON) This would prevent reaching the warning message below.
Hello build bot (Jenkins), David Hendricks, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38833
to look at the new patch set (#4).
Change subject: sb600spi: Add spireadmode ......................................................................
sb600spi: Add spireadmode
Added spireadmode for >= Bolton. Do not override speed or read mode for >= Bolton if parameter not specified. Minor cleanup of sb600spi.c code.
TEST=Manual: deploy on tremblye read flash using various parameters BUG=b:147665085,b:147666328 BRANCH=master
Change-Id: Id7fec7eb87ff811148217dc56a86dca3fef122ff Signed-off-by: Rob Barnes robbarnes@google.com --- M flashrom.8.tmpl M sb600spi.c 2 files changed, 99 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/38833/4
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/flashrom/+/38833/3/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/3/sb600spi.c@453 PS3, Line 453: (amd_gen < CHIPSET_BOLTON)
This would prevent reaching the warning message below.
Good catch!
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/38833/3/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/3/sb600spi.c@453 PS3, Line 453: (amd_gen < CHIPSET_BOLTON)
Good catch!
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 4:
Thanks for taking the time to get this reviewed upstream and thanks Angel for working with Rob to refine this!
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
sb600spi: Add spireadmode
Added spireadmode for >= Bolton. Do not override speed or read mode for >= Bolton if parameter not specified. Minor cleanup of sb600spi.c code.
TEST=Manual: deploy on tremblye read flash using various parameters BUG=b:147665085,b:147666328 BRANCH=master
Change-Id: Id7fec7eb87ff811148217dc56a86dca3fef122ff Signed-off-by: Rob Barnes robbarnes@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/38833 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M flashrom.8.tmpl M sb600spi.c 2 files changed, 99 insertions(+), 44 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index aa5bcd4..fde98c0 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -497,11 +497,29 @@ .sp * SB6xx, SB7xx, SP5xxx: from 16.5 MHz up to and including 33 MHz .sp +-The default is to use 16.5 MHz and disable Fast Reads. +.sp * SB8xx, SB9xx, Hudson: from 16.5 MHz up to and including 66 MHz .sp +-The default is to use 16.5 MHz and disable Fast Reads. +.sp * Yangtze (with SPI 100 engine as found in Kabini and Tamesh): all of them .sp -The default is to use 16.5 MHz and disable Fast Reads. +-The default is to use the frequency that is currently configured. +.sp +An optional +.B spireadmode +parameter specifies the read mode of the SPI bus where applicable (Bolton or later). +Syntax is +.sp +.B " flashrom -p internal:spireadmode=mode" +.sp +where +.B mode +can be +.BR "'Normal\ (up\ to\ 33 MHz)'" ", " "'Normal\ (up\ to\ 66 MHz)'" ", " "'Dual\ IO\ (1-1-2)'" ", " "'Quad\ IO\ (1-1-4)'" ", " "'Dual\ IO\ (1-2-2)'" ", " "'Quad\ IO\ (1-4-4)'" ", or " "'Fast\ Read'" "." +.sp +The default is to use the read mode that is currently configured. .TP .B Intel chipsets .sp diff --git a/sb600spi.c b/sb600spi.c index cb5c4a4..a649253 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -347,23 +347,33 @@ const uint8_t speed; };
-static const struct spispeed spispeeds[] = { - { "66 MHz", 0x00 }, - { "33 MHz", 0x01 }, - { "22 MHz", 0x02 }, - { "16.5 MHz", 0x03 }, - { "100 MHz", 0x04 }, - { "Reserved", 0x05 }, - { "Reserved", 0x06 }, - { "800 kHz", 0x07 }, +static const char* spispeeds[] = { + "66 MHz", + "33 MHz", + "22 MHz", + "16.5 MHz", + "100 MHz", + "Reserved", + "Reserved", + "800 kHz", };
-static int set_speed(struct pci_dev *dev, const struct spispeed *spispeed) +static const char* spireadmodes[] = { + "Normal (up to 33 MHz)", + "Reserved", + "Dual IO (1-1-2)", + "Quad IO (1-1-4)", + "Dual IO (1-2-2)", + "Quad IO (1-4-4)", + "Normal (up to 66 MHz)", + "Fast Read", +}; + +static int set_speed(struct pci_dev *dev, uint8_t speed) { bool success = false; - uint8_t speed = spispeed->speed;
- msg_pdbg("Setting SPI clock to %s (0x%x).\n", spispeed->name, speed); + msg_pdbg("Setting SPI clock to %s (%i)... ", spispeeds[speed], speed); if (amd_gen >= CHIPSET_YANGTZE) { rmmio_writew((speed << 12) | (speed << 8) | (speed << 4) | speed, sb600_spibar + 0x22); uint16_t tmp = mmio_readw(sb600_spibar + 0x22); @@ -375,33 +385,41 @@ }
if (!success) { - msg_perr("Setting SPI clock failed.\n"); + msg_perr("FAILED!\n"); return 1; } + msg_pdbg("succeeded.\n"); return 0; }
-static int set_mode(struct pci_dev *dev, uint8_t read_mode) +static int set_mode(struct pci_dev *dev, uint8_t mode) { + msg_pdbg("Setting SPI read mode to %s (%i)... ", spireadmodes[mode], mode); uint32_t tmp = mmio_readl(sb600_spibar + 0x00); tmp &= ~(0x6 << 28 | 0x1 << 18); /* Clear mode bits */ - tmp |= ((read_mode & 0x6) << 28) | ((read_mode & 0x1) << 18); + tmp |= ((mode & 0x6) << 28) | ((mode & 0x1) << 18); rmmio_writel(tmp, sb600_spibar + 0x00); - if (tmp != mmio_readl(sb600_spibar + 0x00)) + if (tmp != mmio_readl(sb600_spibar + 0x00)) { + msg_perr("FAILED!\n"); return 1; + } + msg_pdbg("succeeded.\n"); return 0; }
static int handle_speed(struct pci_dev *dev) { uint32_t tmp; - uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */ + int16_t spispeed_idx = -1; + int16_t spireadmode_idx = -1; + char *spispeed; + char *spireadmode;
- char *spispeed = extract_programmer_param("spispeed"); + spispeed = extract_programmer_param("spispeed"); if (spispeed != NULL) { unsigned int i; for (i = 0; i < ARRAY_SIZE(spispeeds); i++) { - if (strcasecmp(spispeeds[i].name, spispeed) == 0) { + if (strcasecmp(spispeeds[i], spispeed) == 0) { spispeed_idx = i; break; } @@ -421,32 +439,44 @@ free(spispeed); }
+ spireadmode = extract_programmer_param("spireadmode"); + if (spireadmode != NULL) { + unsigned int i; + for (i = 0; i < ARRAY_SIZE(spireadmodes); i++) { + if (strcasecmp(spireadmodes[i], spireadmode) == 0) { + spireadmode_idx = i; + break; + } + } + if ((strcasecmp(spireadmode, "reserved") == 0) || + (i == ARRAY_SIZE(spireadmodes))) { + msg_perr("Error: Invalid spireadmode value: '%s'.\n", spireadmode); + free(spireadmode); + return 1; + } + if (amd_gen < CHIPSET_BOLTON) { + msg_perr("Warning: spireadmode not supported for this chipset."); + } + free(spireadmode); + } + /* See the chipset support matrix for SPI Base_Addr below for an explanation of the symbols used. * bit 6xx 7xx/SP5100 8xx 9xx hudson1 hudson234 bolton/yangtze * 18 rsvd <- fastReadEnable ? <- ? SpiReadMode[0] * 29:30 rsvd <- <- ? <- ? SpiReadMode[2:1] */ if (amd_gen >= CHIPSET_BOLTON) { - static const char *spireadmodes[] = { - "Normal (up to 33 MHz)", /* 0 */ - "Reserved", /* 1 */ - "Dual IO (1-1-2)", /* 2 */ - "Quad IO (1-1-4)", /* 3 */ - "Dual IO (1-2-2)", /* 4 */ - "Quad IO (1-4-4)", /* 5 */ - "Normal (up to 66 MHz)", /* 6 */ - "Fast Read", /* 7 (Not defined in the Bolton datasheet.) */ - }; + tmp = mmio_readl(sb600_spibar + 0x00); uint8_t read_mode = ((tmp >> 28) & 0x6) | ((tmp >> 18) & 0x1); - msg_pdbg("SpiReadMode=%s (%i)\n", spireadmodes[read_mode], read_mode); - if (read_mode != 6) { - read_mode = 6; /* Default to "Normal (up to 66 MHz)" */ - if (set_mode(dev, read_mode) != 0) { - msg_perr("Setting read mode to "%s" failed.\n", spireadmodes[read_mode]); - return 1; - } - msg_pdbg("Setting read mode to "%s" succeeded.\n", spireadmodes[read_mode]); + msg_pdbg("SPI read mode is %s (%i)\n", + spireadmodes[read_mode], read_mode); + if (spireadmode_idx < 0) { + msg_perr("Warning: spireadmode not set, " + "leaving spireadmode unchanged."); + } + else if (set_mode(dev, spireadmode_idx) != 0) { + return 1; }
if (amd_gen >= CHIPSET_YANGTZE) { @@ -463,10 +493,10 @@ }
tmp = mmio_readw(sb600_spibar + 0x22); /* SPI 100 Speed Config */ - msg_pdbg("NormSpeedNew is %s\n", spispeeds[(tmp >> 12) & 0xf].name); - msg_pdbg("FastSpeedNew is %s\n", spispeeds[(tmp >> 8) & 0xf].name); - msg_pdbg("AltSpeedNew is %s\n", spispeeds[(tmp >> 4) & 0xf].name); - msg_pdbg("TpmSpeedNew is %s\n", spispeeds[(tmp >> 0) & 0xf].name); + msg_pdbg("NormSpeedNew is %s\n", spispeeds[(tmp >> 12) & 0xf]); + msg_pdbg("FastSpeedNew is %s\n", spispeeds[(tmp >> 8) & 0xf]); + msg_pdbg("AltSpeedNew is %s\n", spispeeds[(tmp >> 4) & 0xf]); + msg_pdbg("TpmSpeedNew is %s\n", spispeeds[(tmp >> 0) & 0xf]); } } else { if (amd_gen >= CHIPSET_SB89XX && amd_gen <= CHIPSET_HUDSON234) { @@ -479,9 +509,16 @@ } } tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3; - msg_pdbg("NormSpeed is %s\n", spispeeds[tmp].name); + msg_pdbg("NormSpeed is %s\n", spispeeds[tmp]); + if (spispeed_idx < 0) { + spispeed_idx = 3; /* Default to 16.5 MHz */ + } } - return set_speed(dev, &spispeeds[spispeed_idx]); + if (spispeed_idx < 0) { + msg_perr("Warning: spispeed not set, leaving spispeed unchanged."); + return 0; + } + return set_speed(dev, spispeed_idx); }
static int handle_imc(struct pci_dev *dev)
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
Rob Barnes (Code Review)