Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33040
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
sb/intel/bd82x6x: Use common final SPI OPs setup
This also reworks the interface to override OPs from the devicetree to match the interface in sb/intel/common/spi.
Change-Id: I534e989279d771ec4c0249af325bc3b30a661145 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/sapphire/pureplatinumh61/devicetree.cb M src/southbridge/intel/bd82x6x/chip.h M src/southbridge/intel/bd82x6x/lpc.c 3 files changed, 24 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/33040/1
diff --git a/src/mainboard/sapphire/pureplatinumh61/devicetree.cb b/src/mainboard/sapphire/pureplatinumh61/devicetree.cb index 95c59df..1ad58b8 100644 --- a/src/mainboard/sapphire/pureplatinumh61/devicetree.cb +++ b/src/mainboard/sapphire/pureplatinumh61/devicetree.cb @@ -56,7 +56,14 @@ register "sata_interface_speed_support" = "0x3" register "sata_port_map" = "0x33" register "spi.opprefixes" = "{ 0x50, 0x06 }" - register "spi.ops" = "{ { 0, 1, 0x01 }, { 1, 1, 0x02 }, { 1, 0, 0x03 }, { 0, 0, 0x05 }, { 1, 1, 0x20 }, { 0, 0, 0x9f }, { 0, 1, 0xad }, { 0, 1, 0x04 } }" + register "spi.ops" = "{ {0x01, WRITE_NO_ADDR }, + {0x02, WRITE_WITH_ADDR }, + {0x03, READ_WITH_ADDR }, + {0x05, READ_NO_ADDR }, + {0x20, WRITE_WITH_ADDR }, + {0x9f, READ_NO_ADDR }, + {0xad, WRITE_NO_ADDR }, + {0x04, WRITE_NO_ADDR } }" device pci 16.0 on # Management Engine Interface 1 subsystemid 0x174b 0x1007 end diff --git a/src/southbridge/intel/bd82x6x/chip.h b/src/southbridge/intel/bd82x6x/chip.h index 29f6881..d04cbec 100644 --- a/src/southbridge/intel/bd82x6x/chip.h +++ b/src/southbridge/intel/bd82x6x/chip.h @@ -16,6 +16,7 @@ #ifndef SOUTHBRIDGE_INTEL_BD82X6X_CHIP_H #define SOUTHBRIDGE_INTEL_BD82X6X_CHIP_H
+#include <southbridge/intel/common/spi.h> #include <stdint.h>
struct southbridge_intel_bd82x6x_config { @@ -97,14 +98,7 @@
uint32_t spi_uvscc; uint32_t spi_lvscc; - struct { - uint8_t opprefixes[2]; - struct { - uint8_t needs_address; - uint8_t is_write; - uint8_t op; - } ops[8]; - } spi; + struct intel_spi_config spi; };
#endif /* SOUTHBRIDGE_INTEL_BD82X6X_CHIP_H */ diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c index 7737501..9615698 100644 --- a/src/southbridge/intel/bd82x6x/lpc.c +++ b/src/southbridge/intel/bd82x6x/lpc.c @@ -38,6 +38,7 @@ #include <southbridge/intel/common/acpi_pirq_gen.h> #include <southbridge/intel/common/pmutil.h> #include <southbridge/intel/common/rtc.h> +#include <southbridge/intel/common/spi.h>
#define NMI_OFF 0
@@ -875,34 +876,6 @@
static void lpc_final(struct device *dev) { - u16 spi_opprefix = SPI_OPPREFIX; - u16 spi_optype = SPI_OPTYPE; - u32 spi_opmenu[2] = { SPI_OPMENU_LOWER, SPI_OPMENU_UPPER }; - - /* Configure SPI opcode menu; devicetree may override defaults. */ - const config_t *const config = dev->chip_info; - if (config && config->spi.ops[0].op) { - unsigned int i; - - spi_opprefix = 0; - spi_optype = 0; - spi_opmenu[0] = 0; - spi_opmenu[1] = 0; - for (i = 0; i < sizeof(spi_opprefix); ++i) - spi_opprefix |= config->spi.opprefixes[i] << i * 8; - for (i = 0; i < sizeof(spi_opmenu); ++i) { - spi_optype |= - config->spi.ops[i].is_write << 2 * i | - config->spi.ops[i].needs_address << (2 * i + 1); - spi_opmenu[i / 4] |= - config->spi.ops[i].op << (i % 4) * 8; - } - } - RCBA16(0x3894) = spi_opprefix; - RCBA16(0x3896) = spi_optype; - RCBA32(0x3898) = spi_opmenu[0]; - RCBA32(0x389c) = spi_opmenu[1]; - /* Call SMM finalize() handlers before resume */ if (CONFIG(HAVE_SMI_HANDLER)) { if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || @@ -918,6 +891,19 @@ } }
+void intel_southbridge_override_spi(struct intel_spi_config *spi_config) +{ + struct device *dev = pcidev_on_root(0x1f, 0); + /* Devicetree may override defaults. */ + const config_t *const config = dev->chip_info; + + if (config) + return; + + if (config->spi.ops[0].op != 0) + memcpy(spi_config, &config->spi, sizeof(config->spi)); +} + static struct pci_operations pci_ops = { .set_subsystem = pci_dev_set_subsystem, };
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33040
to look at the new patch set (#3).
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
sb/intel/bd82x6x: Use common final SPI OPs setup
This also reworks the interface to override OPs from the devicetree to match the interface in sb/intel/common/spi.
Change-Id: I534e989279d771ec4c0249af325bc3b30a661145 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/sapphire/pureplatinumh61/devicetree.cb M src/southbridge/intel/bd82x6x/chip.h M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/bd82x6x/pch.h 4 files changed, 24 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/33040/3
Hello Thomas Heijligen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33040
to look at the new patch set (#4).
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
sb/intel/bd82x6x: Use common final SPI OPs setup
This also reworks the interface to override OPs from the devicetree to match the interface in sb/intel/common/spi.
Change-Id: I534e989279d771ec4c0249af325bc3b30a661145 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/sapphire/pureplatinumh61/devicetree.cb M src/southbridge/intel/bd82x6x/chip.h M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/bd82x6x/pch.h 4 files changed, 25 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/33040/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33040/12/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/lpc.c:
https://review.coreboot.org/c/coreboot/+/33040/12/src/southbridge/intel/bd82... PS12, Line 878: { call spi_finalize_ops() ?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 12: Code-Review-1
-1 to call attention to Nico's comment.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 12:
Patch Set 12: Code-Review-1
-1 to call attention to Nico's comment.
FWIW, the 'All-Comments-Resolved' score keeps this from getting accidentially submitted. But it's nice to call for attention
Hello Thomas Heijligen, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33040
to look at the new patch set (#13).
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
sb/intel/bd82x6x: Use common final SPI OPs setup
This also reworks the interface to override OPs from the devicetree to match the interface in sb/intel/common/spi.
Change-Id: I534e989279d771ec4c0249af325bc3b30a661145 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/sapphire/pureplatinumh61/devicetree.cb M src/southbridge/intel/bd82x6x/chip.h M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/bd82x6x/pch.h 4 files changed, 26 insertions(+), 77 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/33040/13
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 13:
(1 comment)
Patch Set 12: Code-Review-1
-1 to call attention to Nico's comment.
https://review.coreboot.org/c/coreboot/+/33040/12/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/lpc.c:
https://review.coreboot.org/c/coreboot/+/33040/12/src/southbridge/intel/bd82... PS12, Line 878: {
call spi_finalize_ops() ?
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... File src/mainboard/sapphire/pureplatinumh61/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... PS13, Line 58: register "spi.ops" = "{ {0x01, WRITE_NO_ADDR }, nit, no space after opening { but before closing }
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/lpc.c:
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... PS13, Line 892: struct device *dev = pcidev_on_root(0x1f, 0); can't this return NULL?
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... PS13, Line 896: if (config) !config I assume
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... PS13, Line 900: memcpy(spi_config, &config->spi, sizeof(config->spi)); please always use sizeof() the target (if they are supposed to be equal)
Hello Patrick Rudolph, Thomas Heijligen, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33040
to look at the new patch set (#14).
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
sb/intel/bd82x6x: Use common final SPI OPs setup
This also reworks the interface to override OPs from the devicetree to match the interface in sb/intel/common/spi.
Change-Id: I534e989279d771ec4c0249af325bc3b30a661145 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/sapphire/pureplatinumh61/devicetree.cb M src/southbridge/intel/bd82x6x/chip.h M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/bd82x6x/pch.h 4 files changed, 29 insertions(+), 77 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/33040/14
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... File src/mainboard/sapphire/pureplatinumh61/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... PS13, Line 58: register "spi.ops" = "{ {0x01, WRITE_NO_ADDR },
nit, no space after opening { but before closing }
Done
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/lpc.c:
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... PS13, Line 892: struct device *dev = pcidev_on_root(0x1f, 0);
can't this return NULL?
Probably not since this is indirectly called by a PCI op for the LPC device, but better safe than sorry I guess...
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... PS13, Line 896: if (config)
!config I assume
Done
https://review.coreboot.org/c/coreboot/+/33040/13/src/southbridge/intel/bd82... PS13, Line 900: memcpy(spi_config, &config->spi, sizeof(config->spi));
please always use sizeof() the target (if they are supposed to be equal)
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 14: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... File src/mainboard/sapphire/pureplatinumh61/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... PS13, Line 58: register "spi.ops" = "{ {0x01, WRITE_NO_ADDR },
Done
Do you by chance remember what flash this ought to support? With the SST detection in place this might be removed?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... File src/mainboard/sapphire/pureplatinumh61/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33040/13/src/mainboard/sapphire/pur... PS13, Line 58: register "spi.ops" = "{ {0x01, WRITE_NO_ADDR },
Do you by chance remember what flash this ought to support? With the SST detection in place this mig […]
I can only see CB:25551
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/33040 )
Change subject: sb/intel/bd82x6x: Use common final SPI OPs setup ......................................................................
sb/intel/bd82x6x: Use common final SPI OPs setup
This also reworks the interface to override OPs from the devicetree to match the interface in sb/intel/common/spi.
Change-Id: I534e989279d771ec4c0249af325bc3b30a661145 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/33040 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/mainboard/sapphire/pureplatinumh61/devicetree.cb M src/southbridge/intel/bd82x6x/chip.h M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/bd82x6x/pch.h 4 files changed, 29 insertions(+), 77 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/mainboard/sapphire/pureplatinumh61/devicetree.cb b/src/mainboard/sapphire/pureplatinumh61/devicetree.cb index aff0130..b863c308 100644 --- a/src/mainboard/sapphire/pureplatinumh61/devicetree.cb +++ b/src/mainboard/sapphire/pureplatinumh61/devicetree.cb @@ -55,7 +55,14 @@ register "sata_interface_speed_support" = "0x3" register "sata_port_map" = "0x33" register "spi.opprefixes" = "{ 0x50, 0x06 }" - register "spi.ops" = "{ { 0, 1, 0x01 }, { 1, 1, 0x02 }, { 1, 0, 0x03 }, { 0, 0, 0x05 }, { 1, 1, 0x20 }, { 0, 0, 0x9f }, { 0, 1, 0xad }, { 0, 1, 0x04 } }" + register "spi.ops" = "{{0x01, WRITE_NO_ADDR}, + {0x02, WRITE_WITH_ADDR}, + {0x03, READ_WITH_ADDR}, + {0x05, READ_NO_ADDR}, + {0x20, WRITE_WITH_ADDR}, + {0x9f, READ_NO_ADDR}, + {0xad, WRITE_NO_ADDR}, + {0x04, WRITE_NO_ADDR}}" device pci 16.0 on # Management Engine Interface 1 subsystemid 0x174b 0x1007 end diff --git a/src/southbridge/intel/bd82x6x/chip.h b/src/southbridge/intel/bd82x6x/chip.h index 4be9152..9f9c445 100644 --- a/src/southbridge/intel/bd82x6x/chip.h +++ b/src/southbridge/intel/bd82x6x/chip.h @@ -16,6 +16,7 @@ #ifndef SOUTHBRIDGE_INTEL_BD82X6X_CHIP_H #define SOUTHBRIDGE_INTEL_BD82X6X_CHIP_H
+#include <southbridge/intel/common/spi.h> #include <stdint.h>
struct southbridge_intel_bd82x6x_config { @@ -96,14 +97,7 @@
uint32_t spi_uvscc; uint32_t spi_lvscc; - struct { - uint8_t opprefixes[2]; - struct { - uint8_t needs_address; - uint8_t is_write; - uint8_t op; - } ops[8]; - } spi; + struct intel_swseq_spi_config spi; };
#endif /* SOUTHBRIDGE_INTEL_BD82X6X_CHIP_H */ diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c index 592c70f..b8df7aa 100644 --- a/src/southbridge/intel/bd82x6x/lpc.c +++ b/src/southbridge/intel/bd82x6x/lpc.c @@ -39,6 +39,7 @@ #include <southbridge/intel/common/acpi_pirq_gen.h> #include <southbridge/intel/common/pmutil.h> #include <southbridge/intel/common/rtc.h> +#include <southbridge/intel/common/spi.h>
#define NMI_OFF 0
@@ -874,33 +875,7 @@
static void lpc_final(struct device *dev) { - u16 spi_opprefix = SPI_OPPREFIX; - u16 spi_optype = SPI_OPTYPE; - u32 spi_opmenu[2] = { SPI_OPMENU_LOWER, SPI_OPMENU_UPPER }; - - /* Configure SPI opcode menu; devicetree may override defaults. */ - const config_t *const config = dev->chip_info; - if (config && config->spi.ops[0].op) { - unsigned int i; - - spi_opprefix = 0; - spi_optype = 0; - spi_opmenu[0] = 0; - spi_opmenu[1] = 0; - for (i = 0; i < sizeof(spi_opprefix); ++i) - spi_opprefix |= config->spi.opprefixes[i] << i * 8; - for (i = 0; i < sizeof(spi_opmenu); ++i) { - spi_optype |= - config->spi.ops[i].is_write << 2 * i | - config->spi.ops[i].needs_address << (2 * i + 1); - spi_opmenu[i / 4] |= - config->spi.ops[i].op << (i % 4) * 8; - } - } - RCBA16(0x3894) = spi_opprefix; - RCBA16(0x3896) = spi_optype; - RCBA32(0x3898) = spi_opmenu[0]; - RCBA32(0x389c) = spi_opmenu[1]; + spi_finalize_ops();
/* Call SMM finalize() handlers before resume */ if (CONFIG(HAVE_SMI_HANDLER)) { @@ -911,6 +886,23 @@ } }
+void intel_southbridge_override_spi( + struct intel_swseq_spi_config *spi_config) +{ + struct device *dev = pcidev_on_root(0x1f, 0); + + if (!dev) + return; + /* Devicetree may override defaults. */ + const config_t *const config = dev->chip_info; + + if (!config) + return; + + if (config->spi.ops[0].op != 0) + memcpy(spi_config, &config->spi, sizeof(*spi_config)); +} + static struct pci_operations pci_ops = { .set_subsystem = pci_dev_set_subsystem, }; diff --git a/src/southbridge/intel/bd82x6x/pch.h b/src/southbridge/intel/bd82x6x/pch.h index fcb15ac..cb0691f 100644 --- a/src/southbridge/intel/bd82x6x/pch.h +++ b/src/southbridge/intel/bd82x6x/pch.h @@ -561,47 +561,6 @@ #define TCO_LOCK (1 << 12) #define TCO2_CNT 0x6a
-/* - * SPI Opcode Menu setup for SPIBAR lockdown - * should support most common flash chips. - */ - -#define SPI_OPMENU_0 0x01 /* WRSR: Write Status Register */ -#define SPI_OPTYPE_0 0x01 /* Write, no address */ - -#define SPI_OPMENU_1 0x02 /* BYPR: Byte Program */ -#define SPI_OPTYPE_1 0x03 /* Write, address required */ - -#define SPI_OPMENU_2 0x03 /* READ: Read Data */ -#define SPI_OPTYPE_2 0x02 /* Read, address required */ - -#define SPI_OPMENU_3 0x05 /* RDSR: Read Status Register */ -#define SPI_OPTYPE_3 0x00 /* Read, no address */ - -#define SPI_OPMENU_4 0x20 /* SE20: Sector Erase 0x20 */ -#define SPI_OPTYPE_4 0x03 /* Write, address required */ - -#define SPI_OPMENU_5 0x9f /* RDID: Read ID */ -#define SPI_OPTYPE_5 0x00 /* Read, no address */ - -#define SPI_OPMENU_6 0xd8 /* BED8: Block Erase 0xd8 */ -#define SPI_OPTYPE_6 0x03 /* Write, address required */ - -#define SPI_OPMENU_7 0x0b /* FAST: Fast Read */ -#define SPI_OPTYPE_7 0x02 /* Read, address required */ - -#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) | \ - (SPI_OPMENU_5 << 8) | SPI_OPMENU_4) -#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) | \ - (SPI_OPMENU_1 << 8) | SPI_OPMENU_0) - -#define SPI_OPTYPE ((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \ - (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 << 8) | \ - (SPI_OPTYPE_3 << 6) | (SPI_OPTYPE_2 << 4) | \ - (SPI_OPTYPE_1 << 2) | (SPI_OPTYPE_0)) - -#define SPI_OPPREFIX ((0x50 << 8) | 0x06) /* EWSR and WREN */ - #define SPIBAR_HSFS 0x3804 /* SPI hardware sequence status */ #define SPIBAR_HSFS_SCIP (1 << 5) /* SPI Cycle In Progress */ #define SPIBAR_HSFS_AEL (1 << 2) /* SPI Access Error Log */