Change in coreboot[master]: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
soc/intel/broadwell: Drop reg-script usage from bootblock PCH init
Change-Id: I87145215ccec86e391d0dbd9171b08d7fd73ad9a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/broadwell/bootblock/pch.c 1 file changed, 50 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/46352/1
diff --git a/src/soc/intel/broadwell/bootblock/pch.c b/src/soc/intel/broadwell/bootblock/pch.c index 27d9a3e..14a5061 100644 --- a/src/soc/intel/broadwell/bootblock/pch.c +++ b/src/soc/intel/broadwell/bootblock/pch.c @@ -7,7 +7,6 @@ #include <soc/pci_devs.h> #include <soc/rcba.h> #include <soc/spi.h> -#include <reg_script.h> #include <soc/pm.h> #include <soc/romstage.h> #include <southbridge/intel/common/early_spi.h> @@ -46,49 +45,61 @@ SPIBAR8(SPIBAR_SSFC + 2) = ssfc; }
-const struct reg_script pch_early_init_script[] = { - /* Setup southbridge BARs */ - REG_PCI_WRITE32(RCBA, RCBA_BASE_ADDRESS | 1), - REG_PCI_WRITE32(PMBASE, ACPI_BASE_ADDRESS | 1), - REG_PCI_WRITE8(ACPI_CNTL, ACPI_EN), - REG_PCI_WRITE32(GPIO_BASE, GPIO_BASE_ADDRESS | 1), - REG_PCI_WRITE8(GPIO_CNTL, GPIO_EN), +static void pch_enable_bars(void) +{ + /* Set up southbridge BARs */ + pci_write_config32(PCH_DEV_LPC, RCBA, RCBA_BASE_ADDRESS | 1); + + pci_write_config32(PCH_DEV_LPC, PMBASE, ACPI_BASE_ADDRESS | 1); + + pci_write_config8(PCH_DEV_LPC, ACPI_CNTL, ACPI_EN); + + pci_write_config32(PCH_DEV_LPC, GPIO_BASE, GPIO_BASE_ADDRESS | 1); + + /* Enable GPIO functionality. */ + pci_write_config8(PCH_DEV_LPC, GPIO_CNTL, GPIO_EN);
/* Set COM1/COM2 decode range */ - REG_PCI_WRITE16(LPC_IO_DEC, 0x0010), - /* Enable legacy decode ranges */ - REG_PCI_WRITE16(LPC_EN, CNF1_LPC_EN | CNF2_LPC_EN | GAMEL_LPC_EN | - COMA_LPC_EN | KBC_LPC_EN | MC_LPC_EN), - - /* Enable IOAPIC */ - REG_MMIO_WRITE16(RCBA_BASE_ADDRESS + OIC, 0x0100), - /* Read back for posted write */ - REG_MMIO_READ16(RCBA_BASE_ADDRESS + OIC), - - /* Set HPET address and enable it */ - REG_MMIO_RMW32(RCBA_BASE_ADDRESS + HPTC, ~3, (1 << 7)), - /* Read back for posted write */ - REG_MMIO_READ32(RCBA_BASE_ADDRESS + HPTC), - /* Enable HPET to start counter */ - REG_MMIO_OR32(HPET_BASE_ADDRESS + 0x10, (1 << 0)), - - /* Disable reset */ - REG_MMIO_OR32(RCBA_BASE_ADDRESS + GCS, (1 << 5)), - /* TCO timer halt */ - REG_IO_OR16(ACPI_BASE_ADDRESS + TCO1_CNT, TCO_TMR_HLT), - - /* Enable upper 128 bytes of CMOS */ - REG_MMIO_OR32(RCBA_BASE_ADDRESS + RC, (1 << 2)), - - /* Disable unused device (always) */ - REG_MMIO_OR32(RCBA_BASE_ADDRESS + FD, PCH_DISABLE_ALWAYS), - - REG_SCRIPT_END -}; + pci_write_config16(PCH_DEV_LPC, LPC_IO_DEC, 0x0010); +}
static void pch_early_lpc(void) { - reg_script_run_on_dev(PCH_DEV_LPC, pch_early_init_script); + pch_enable_bars(); + + /* Enable SuperIO + MC + COM1 + PS/2 Keyboard/Mouse */ + u16 lpc_config = CNF1_LPC_EN | CNF2_LPC_EN | GAMEL_LPC_EN | + COMA_LPC_EN | KBC_LPC_EN | MC_LPC_EN; + pci_write_config16(PCH_DEV_LPC, LPC_EN, lpc_config); + + /* Enable IOAPIC */ + RCBA16(OIC) = 0x0100; + + /* Read back for posted write */ + (void) RCBA16(OIC); + + /* Set HPET address and enable it */ + RCBA32_AND_OR(HPTC, ~3, 1 << 7); + + /* Read it back to stick. It's affected by posted write syndrome. */ + RCBA32(HPTC); + + /* Enable HPET to start counter */ + setbits32((void *)HPET_BASE_ADDRESS + 0x10, 1 << 0); + + /* Disable reset */ + RCBA32_OR(GCS, 1 << 5); + + /* TCO timer halt */ + u16 reg16 = inb(ACPI_BASE_ADDRESS + TCO1_CNT); + reg16 |= TCO_TMR_HLT; + outb(reg16, ACPI_BASE_ADDRESS + TCO1_CNT); + + /* Enable upper 128 bytes of CMOS */ + RCBA32_OR(RC, 1 << 2); + + /* Disable unused device (always) */ + RCBA32_OR(FD, PCH_DISABLE_ALWAYS); }
void bootblock_early_southbridge_init(void)
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 82: RCBA32_AND_OR(HPTC, ~3, 1 << 7); overflow if some one add new macro for 16 or 8....
I think this is no a good idea
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 82: RCBA32_AND_OR(HPTC, ~3, 1 << 7);
overflow if some one add new macro for 16 or 8.... […]
Nak
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 82: RCBA32_AND_OR(HPTC, ~3, 1 << 7);
Nak
Elyes, what you seem to predict there doesn't make any sense. Also, any overflow would be intentional. You probably refer to the spurious compiler warnings that we don't enable? Please do not block coreboot development because of potential future issue that can be avoided in one way or another.
The macro still exists and it's safe to be used like that. You should know that or not add blocking comments. You can always mark comments as resolved if you are not 100% sure that things should be blocked.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 59: /* Enable GPIO functionality. */ Want to comment the other lines too? (kidding)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 63: pci_write_config16(PCH_DEV_LPC, LPC_IO_DEC, 0x0010); This doesn't look like BAR setup.
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 79: Spurious space
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 84: It's affected by posted write syndrome. That's neither helpful for people who understand posting writes nor for those who don't.
How about something like this:
/* Reading it back guarantees that the write is done before we use the configured base address below. */
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 85: RCBA32(HPTC); Why use a (void) cast above to make it clear that the value is discarded but not here?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 63: pci_write_config16(PCH_DEV_LPC, LPC_IO_DEC, 0x0010);
This doesn't look like BAR setup.
Oops, slipped through. Will move out of this function
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 79:
Spurious space
Was there on Lynxpoint, will drop
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46352
to look at the new patch set (#5).
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
soc/intel/broadwell: Drop reg-script usage from bootblock PCH init
Change-Id: I87145215ccec86e391d0dbd9171b08d7fd73ad9a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/broadwell/bootblock/pch.c 1 file changed, 51 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/46352/5
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46352
to look at the new patch set (#7).
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
soc/intel/broadwell: Drop reg-script usage from bootblock PCH init
Change-Id: I87145215ccec86e391d0dbd9171b08d7fd73ad9a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/broadwell/bootblock/pch.c 1 file changed, 50 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/46352/7
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46352
to look at the new patch set (#8).
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
soc/intel/broadwell: Drop reg-script usage from bootblock PCH init
Change-Id: I87145215ccec86e391d0dbd9171b08d7fd73ad9a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/broadwell/bootblock/pch.c 1 file changed, 51 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/46352/8
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 63: pci_write_config16(PCH_DEV_LPC, LPC_IO_DEC, 0x0010);
Oops, slipped through. […]
Done
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 79:
Was there on Lynxpoint, will drop
Done
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 84: It's affected by posted write syndrome.
That's neither helpful for people who understand posting writes nor for […]
Done
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 85: RCBA32(HPTC);
Why use a (void) cast above to make it clear that the value is discarded […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
Patch Set 8: Code-Review+2
(6 comments)
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 63: pci_write_config16(PCH_DEV_LPC, LPC_IO_DEC, 0x0010);
Oops, slipped through. […]
Done
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 79:
Was there on Lynxpoint, will drop
Done
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 84: It's affected by posted write syndrome.
That's neither helpful for people who understand posting writes nor for […]
Done
https://review.coreboot.org/c/coreboot/+/46352/2/src/soc/intel/broadwell/boo... PS2, Line 85: RCBA32(HPTC);
Why use a (void) cast above to make it clear that the value is discarded […]
Done
https://review.coreboot.org/c/coreboot/+/46352/8/src/soc/intel/broadwell/boo... File src/soc/intel/broadwell/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/46352/8/src/soc/intel/broadwell/boo... PS8, Line 70: /* Enable SuperIO + MC + COM1 + PS/2 Keyboard/Mouse */ GAMEL not mentioned?
https://review.coreboot.org/c/coreboot/+/46352/8/src/soc/intel/broadwell/boo... PS8, Line 91: (void *)HPET_BASE_ADDRESS + 0x10 Technically this is undefined (but we know what GCC will do). The cast takes precedence and then you do pointer arithmetic on a void pointer.
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46352 )
Change subject: soc/intel/broadwell: Drop reg-script usage from bootblock PCH init ......................................................................
soc/intel/broadwell: Drop reg-script usage from bootblock PCH init
Change-Id: I87145215ccec86e391d0dbd9171b08d7fd73ad9a Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46352 Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/broadwell/bootblock/pch.c 1 file changed, 51 insertions(+), 37 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/soc/intel/broadwell/bootblock/pch.c b/src/soc/intel/broadwell/bootblock/pch.c index 27d9a3e..7f6d0d5 100644 --- a/src/soc/intel/broadwell/bootblock/pch.c +++ b/src/soc/intel/broadwell/bootblock/pch.c @@ -7,7 +7,6 @@ #include <soc/pci_devs.h> #include <soc/rcba.h> #include <soc/spi.h> -#include <reg_script.h> #include <soc/pm.h> #include <soc/romstage.h> #include <southbridge/intel/common/early_spi.h> @@ -46,49 +45,64 @@ SPIBAR8(SPIBAR_SSFC + 2) = ssfc; }
-const struct reg_script pch_early_init_script[] = { - /* Setup southbridge BARs */ - REG_PCI_WRITE32(RCBA, RCBA_BASE_ADDRESS | 1), - REG_PCI_WRITE32(PMBASE, ACPI_BASE_ADDRESS | 1), - REG_PCI_WRITE8(ACPI_CNTL, ACPI_EN), - REG_PCI_WRITE32(GPIO_BASE, GPIO_BASE_ADDRESS | 1), - REG_PCI_WRITE8(GPIO_CNTL, GPIO_EN), +static void pch_enable_bars(void) +{ + /* Set up southbridge BARs */ + pci_write_config32(PCH_DEV_LPC, RCBA, RCBA_BASE_ADDRESS | 1);
- /* Set COM1/COM2 decode range */ - REG_PCI_WRITE16(LPC_IO_DEC, 0x0010), - /* Enable legacy decode ranges */ - REG_PCI_WRITE16(LPC_EN, CNF1_LPC_EN | CNF2_LPC_EN | GAMEL_LPC_EN | - COMA_LPC_EN | KBC_LPC_EN | MC_LPC_EN), + pci_write_config32(PCH_DEV_LPC, PMBASE, ACPI_BASE_ADDRESS | 1);
- /* Enable IOAPIC */ - REG_MMIO_WRITE16(RCBA_BASE_ADDRESS + OIC, 0x0100), - /* Read back for posted write */ - REG_MMIO_READ16(RCBA_BASE_ADDRESS + OIC), + pci_write_config8(PCH_DEV_LPC, ACPI_CNTL, ACPI_EN);
- /* Set HPET address and enable it */ - REG_MMIO_RMW32(RCBA_BASE_ADDRESS + HPTC, ~3, (1 << 7)), - /* Read back for posted write */ - REG_MMIO_READ32(RCBA_BASE_ADDRESS + HPTC), - /* Enable HPET to start counter */ - REG_MMIO_OR32(HPET_BASE_ADDRESS + 0x10, (1 << 0)), + pci_write_config32(PCH_DEV_LPC, GPIO_BASE, GPIO_BASE_ADDRESS | 1);
- /* Disable reset */ - REG_MMIO_OR32(RCBA_BASE_ADDRESS + GCS, (1 << 5)), - /* TCO timer halt */ - REG_IO_OR16(ACPI_BASE_ADDRESS + TCO1_CNT, TCO_TMR_HLT), - - /* Enable upper 128 bytes of CMOS */ - REG_MMIO_OR32(RCBA_BASE_ADDRESS + RC, (1 << 2)), - - /* Disable unused device (always) */ - REG_MMIO_OR32(RCBA_BASE_ADDRESS + FD, PCH_DISABLE_ALWAYS), - - REG_SCRIPT_END -}; + /* Enable GPIO functionality. */ + pci_write_config8(PCH_DEV_LPC, GPIO_CNTL, GPIO_EN); +}
static void pch_early_lpc(void) { - reg_script_run_on_dev(PCH_DEV_LPC, pch_early_init_script); + pch_enable_bars(); + + /* Set COM1/COM2 decode range */ + pci_write_config16(PCH_DEV_LPC, LPC_IO_DEC, 0x0010); + + /* Enable SuperIO + MC + COM1 + PS/2 Keyboard/Mouse */ + u16 lpc_config = CNF1_LPC_EN | CNF2_LPC_EN | GAMEL_LPC_EN | + COMA_LPC_EN | KBC_LPC_EN | MC_LPC_EN; + pci_write_config16(PCH_DEV_LPC, LPC_EN, lpc_config); + + /* Enable IOAPIC */ + RCBA16(OIC) = 0x0100; + + /* Read back for posted write */ + (void)RCBA16(OIC); + + /* Set HPET address and enable it */ + RCBA32_AND_OR(HPTC, ~3, 1 << 7); + + /* + * Reading the register back guarantees that the write is + * done before we use the configured base address below. + */ + (void)RCBA32(HPTC); + + /* Enable HPET to start counter */ + setbits32((void *)HPET_BASE_ADDRESS + 0x10, 1 << 0); + + /* Disable reset */ + RCBA32_OR(GCS, 1 << 5); + + /* TCO timer halt */ + u16 reg16 = inb(ACPI_BASE_ADDRESS + TCO1_CNT); + reg16 |= TCO_TMR_HLT; + outb(reg16, ACPI_BASE_ADDRESS + TCO1_CNT); + + /* Enable upper 128 bytes of CMOS */ + RCBA32_OR(RC, 1 << 2); + + /* Disable unused device (always) */ + RCBA32_OR(FD, PCH_DISABLE_ALWAYS); }
void bootblock_early_southbridge_init(void)
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)