Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32062
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
soc/intel/braswell/smbus: Init SMBus
Using Intel southbridge common implementation to retrieve SPD from DIMMs causes FSP memory init to hang. Initialize SMBus as in Intel SoC common before issuing any transactions to let FSP properly initialize memory. Also make Intel common southbridge smbus API compatible with SPD library.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I92a2c5a6d0b38e5658cfdc017041f12717dabdd5 --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/romstage/romstage.c A src/soc/intel/braswell/smbus.c 4 files changed, 92 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/32062/1
diff --git a/src/soc/intel/braswell/Makefile.inc b/src/soc/intel/braswell/Makefile.inc index a538f7d..554af41 100644 --- a/src/soc/intel/braswell/Makefile.inc +++ b/src/soc/intel/braswell/Makefile.inc @@ -14,6 +14,7 @@ romstage-y += lpc_init.c romstage-y += memmap.c romstage-y += pmutil.c +romstage-y += smbus.c romstage-y += tsc_freq.c
postcar-y += tsc_freq.c @@ -38,6 +39,7 @@ ramstage-y += sata.c ramstage-y += scc.c ramstage-y += sd.c +ramstage-y += smbus.c ramstage-y += smm.c ramstage-y += southcluster.c ramstage-y += spi.c diff --git a/src/soc/intel/braswell/include/soc/smbus.h b/src/soc/intel/braswell/include/soc/smbus.h new file mode 100644 index 0000000..f5075e1 --- /dev/null +++ b/src/soc/intel/braswell/include/soc/smbus.h @@ -0,0 +1,36 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 3mdeb + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + + +#ifndef _SOC_SMBUS_H_ +#define _SOC_SMBUS_H_ + +#include <soc/pci_devs.h> + +#if !defined(__SIMPLE_DEVICE__) +#include <device/device.h> +#define PCH_DEV_SMBUS dev_find_slot(0, PCI_DEVFN(SMBUS_DEV, SMBUS_FUNC)) +#else + +#include <device/pci_type.h> +#define PCH_DEV_SMBUS PCI_DEV(0, SMBUS_DEV, SMBUS_FUNC) +#endif + +#define HOSTC 0x40 +#define HST_EN (1 << 0) + +void smbus_common_init(void); + +#endif /* _SOC_SMBUS_H_ */ diff --git a/src/soc/intel/braswell/romstage/romstage.c b/src/soc/intel/braswell/romstage/romstage.c index ca1eb40..6a0226e 100644 --- a/src/soc/intel/braswell/romstage/romstage.c +++ b/src/soc/intel/braswell/romstage/romstage.c @@ -40,6 +40,7 @@ #include <soc/lpc.h> #include <soc/pci_devs.h> #include <soc/romstage.h> +#include <soc/smbus.h> #include <soc/smm.h> #include <soc/spi.h> #include <build.h> @@ -194,6 +195,7 @@ spi_init();
lpc_init(); + smbus_common_init(); }
/* SOC initialization after RAM is enabled */ diff --git a/src/soc/intel/braswell/smbus.c b/src/soc/intel/braswell/smbus.c new file mode 100644 index 0000000..454c3f6 --- /dev/null +++ b/src/soc/intel/braswell/smbus.c @@ -0,0 +1,52 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2017 Intel Corporation. + * Copyright (C) 2019 3mdeb + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/early_smbus.h> +#include <device/pci_def.h> +#include <reg_script.h> +#include <soc/iomap.h> +#include <soc/smbus.h> +#include <southbridge/intel/common/smbus.h> + +static const struct reg_script smbus_init_script[] = { + /* Set SMBus I/O base address */ + REG_PCI_WRITE32(PCI_BASE_ADDRESS_4, SMBUS_BASE_ADDRESS), + /* Set SMBus enable */ + REG_PCI_WRITE8(HOSTC, HST_EN), + /* Enable I/O access */ + REG_PCI_WRITE16(PCI_COMMAND, PCI_COMMAND_IO), + /* Disable interrupts */ + REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTCTL, 0), + /* Clear errors */ + REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTSTAT, 0xff), + /* Indicate the end of this array by REG_SCRIPT_END */ + REG_SCRIPT_END, +}; + +u8 smbus_read_byte(u32 smbus_dev, u8 addr, u8 offset) +{ + return do_smbus_read_byte(SMBUS_BASE_ADDRESS, addr, offset); +} + +u8 smbus_write_byte(u32 smbus_dev, u8 addr, u8 offset, u8 value) +{ + return do_smbus_write_byte(SMBUS_BASE_ADDRESS, addr, offset, value); +} + +void smbus_common_init(void) +{ + reg_script_run_on_dev(PCH_DEV_SMBUS, smbus_init_script); +}
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32062/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32062/1//COMMIT_MSG@12 PS1, Line 12: smbus SMBus
Hello Patrick Rudolph, Frans Hendriks, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32062
to look at the new patch set (#2).
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
soc/intel/braswell/smbus: Init SMBus
Using Intel southbridge common implementation to retrieve SPD from DIMMs causes FSP memory init to hang. Initialize SMBus as in Intel SoC common before issuing any transactions to let FSP properly initialize memory. Also make Intel southbridge common SMBus API compatible with SPD library.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I92a2c5a6d0b38e5658cfdc017041f12717dabdd5 --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/romstage/romstage.c A src/soc/intel/braswell/smbus.c 4 files changed, 92 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/32062/2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32062/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32062/1//COMMIT_MSG@12 PS1, Line 12: smbus
SMBus
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32062/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32062/2//COMMIT_MSG@12 PS2, Line 12: Also make Intel southbridge common SMBus API compatible with SPD library. Suggested change of last line: Also make SMBus support compatible with SPD libary using Intel SB common SMBus API.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32062/2/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/32062/2/src/soc/intel/braswell/include/soc/s... PS2, Line 31: #define HOSTC 0x40 : #define HST_EN (1 << 0) These defines are available in soc\intel\common\block\smbus\smbuslib.h. Any reason for 'redefinition' here?
Hello Patrick Rudolph, Frans Hendriks, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32062
to look at the new patch set (#3).
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
soc/intel/braswell/smbus: Init SMBus
Using Intel southbridge common implementation to retrieve SPD from DIMMs causes FSP memory init to hang. Initialize SMBus as in Intel SoC common before issuing any transactions to let FSP properly initialize memory. Also make SMBus support compatible with SPD libary using Intel SB common SMBus API.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I92a2c5a6d0b38e5658cfdc017041f12717dabdd5 --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/romstage/romstage.c A src/soc/intel/braswell/smbus.c 4 files changed, 89 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/32062/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/32062/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32062/2//COMMIT_MSG@12 PS2, Line 12: Also make Intel southbridge common SMBus API compatible with SPD library.
Suggested change of last line: […]
Done
https://review.coreboot.org/#/c/32062/2/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/32062/2/src/soc/intel/braswell/include/soc/s... PS2, Line 31: #define HOSTC 0x40 : #define HST_EN (1 << 0)
These defines are available in soc\intel\common\block\smbus\smbuslib.h. […]
Did not want to mix Intel SoC common and Intel southbridge common, I thought it could introduce some kind of confusion. But no reason to duplicate defines like that.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 3: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc File src/soc/intel/braswell/Makefile.inc:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc@... PS3, Line 42: ramstage-y += smbus.c I don't see how this could work in ramstage. The PCI device is visible to the allocator, which will likely reasign a different i/o address. Then the static addresses in `smbus.c` wouldn't be valid any longer.
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... PS3, Line 15: nit: single empty line, please
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... PS3, Line 24: #define PCH_DEV_SMBUS dev_find_slot(0, PCI_DEVFN(SMBUS_DEV, SMBUS_FUNC)) I don't think we actually need this. You don't seem to be using the SMBus in ramstage and I doubt it's working (see comment in `Makefile.inc`).
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@18 PS3, Line 18: #include <device/pci_def.h> is this needed?
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@22 PS3, Line 22: #include <soc/intel/common/block/smbus/smbuslib.h> it's really confusing
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@32 PS3, Line 32: /* Disable interrupts */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTCTL, 0), : /* Clear errors */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTSTAT, 0xff), The sb/common driver should handle these.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Init SMBus ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc File src/soc/intel/braswell/Makefile.inc:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc@... PS3, Line 42: ramstage-y += smbus.c
I don't see how this could work in ramstage. The PCI device is […]
Since I will use it for SPD retrieval only, I may remove it from ramstage
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@18 PS3, Line 18: #include <device/pci_def.h>
is this needed?
Yes, for the PCI_BASE_ADDRESS_4, PCI_COMMAND and PCI_COMMAND_IO values
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@32 PS3, Line 32: /* Disable interrupts */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTCTL, 0), : /* Clear errors */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTSTAT, 0xff),
The sb/common driver should handle these.
Yes, it clears the errors and interrupts on command setup. I will test without these.
Hello Patrick Rudolph, Frans Hendriks, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32062
to look at the new patch set (#4).
Change subject: soc/intel/braswell/smbus: Enable early SMBus in romstage ......................................................................
soc/intel/braswell/smbus: Enable early SMBus in romstage
Enable early SMBus support compatible with SPD library using Intel SB common SMBus API.
TEST=boot Protectli FW2B with new FSP, MemoryInit should pass without errors
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I92a2c5a6d0b38e5658cfdc017041f12717dabdd5 --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/smbus.c 2 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/32062/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Enable early SMBus in romstage ......................................................................
Patch Set 4:
With the new BSW FSP release the problem with hanging MemoryInit seems to be gone. I have limited the patch to early SMBus and SPD lib compatibility.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Enable early SMBus in romstage ......................................................................
Patch Set 4: Code-Review+2
(5 comments)
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc File src/soc/intel/braswell/Makefile.inc:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/Makefile.inc@... PS3, Line 42: ramstage-y += smbus.c
Since I will use it for SPD retrieval only, I may remove it from ramstage
Ack
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... PS3, Line 15:
nit: single empty line, please
Done
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/include/soc/s... PS3, Line 24: #define PCH_DEV_SMBUS dev_find_slot(0, PCI_DEVFN(SMBUS_DEV, SMBUS_FUNC))
I don't think we actually need this. You don't seem to be using the […]
Done
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@18 PS3, Line 18: #include <device/pci_def.h>
Yes, for the PCI_BASE_ADDRESS_4, PCI_COMMAND and PCI_COMMAND_IO values
Oh, I'm stupid :-/
https://review.coreboot.org/#/c/32062/3/src/soc/intel/braswell/smbus.c@32 PS3, Line 32: /* Disable interrupts */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTCTL, 0), : /* Clear errors */ : REG_IO_WRITE8(SMBUS_BASE_ADDRESS + SMBHSTSTAT, 0xff),
Yes, it clears the errors and interrupts on command setup. I will test without these.
Ack
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Enable early SMBus in romstage ......................................................................
Patch Set 5: Code-Review+1
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Enable early SMBus in romstage ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32062 )
Change subject: soc/intel/braswell/smbus: Enable early SMBus in romstage ......................................................................
soc/intel/braswell/smbus: Enable early SMBus in romstage
Enable early SMBus support compatible with SPD library using Intel SB common SMBus API.
TEST=boot Protectli FW2B with new FSP, MemoryInit should pass without errors
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I92a2c5a6d0b38e5658cfdc017041f12717dabdd5 Reviewed-on: https://review.coreboot.org/c/coreboot/+/32062 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Frans Hendriks fhendriks@eltan.com Reviewed-by: Patrick Georgi pgeorgi@google.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/smbus.c 2 files changed, 30 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Nico Huber: Looks good to me, approved Frans Hendriks: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/braswell/Makefile.inc b/src/soc/intel/braswell/Makefile.inc index a2b7ee5..4e90edf 100644 --- a/src/soc/intel/braswell/Makefile.inc +++ b/src/soc/intel/braswell/Makefile.inc @@ -14,6 +14,7 @@ romstage-y += lpc_init.c romstage-y += memmap.c romstage-y += pmutil.c +romstage-y += smbus.c romstage-y += tsc_freq.c
postcar-y += tsc_freq.c diff --git a/src/soc/intel/braswell/smbus.c b/src/soc/intel/braswell/smbus.c new file mode 100644 index 0000000..7e1b0df --- /dev/null +++ b/src/soc/intel/braswell/smbus.c @@ -0,0 +1,29 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2017 Intel Corporation. + * Copyright (C) 2019 3mdeb + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/early_smbus.h> +#include <soc/iomap.h> +#include <southbridge/intel/common/smbus.h> + +u8 smbus_read_byte(u32 smbus_dev, u8 addr, u8 offset) +{ + return do_smbus_read_byte(SMBUS_BASE_ADDRESS, addr, offset); +} + +u8 smbus_write_byte(u32 smbus_dev, u8 addr, u8 offset, u8 value) +{ + return do_smbus_write_byte(SMBUS_BASE_ADDRESS, addr, offset, value); +}
-
Frans Hendriks (Code Review) -
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)