Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31661
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
soc/intel/braswell: Add SMBus support
Intel Braswell SoC contains SMBus controller but not support is available for this controller. This controller is compatible with the Intel SMBus support in the southbridge directory. This smbus.c file in that directory is included. This smbus.c file can be included in build using config SOUTHBRIDGE_INTEL_COMMON and SOUTHBRIDGE_INTEL_COMMON_SMBUS also. This results into build errors, caused by pmbase.c and rtc.c which is include always..
BUG=N/A TEST= Facebook FBG-1710 LCD panel
Change-Id: Ie3d4f657558a1aed21b083ef5cad08ea96e629c3 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/31661/1
diff --git a/src/soc/intel/braswell/Makefile.inc b/src/soc/intel/braswell/Makefile.inc index d5fe1ab..c7d0c6e 100644 --- a/src/soc/intel/braswell/Makefile.inc +++ b/src/soc/intel/braswell/Makefile.inc @@ -39,6 +39,7 @@ ramstage-y += sata.c ramstage-y += scc.c ramstage-y += sd.c +ramstage-y += ../../../southbridge/intel/common/smbus.c ramstage-y += smm.c ramstage-y += southcluster.c ramstage-y += spi.c
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 1:
(3 comments)
Can you please paste the build error in a comment here?
https://review.coreboot.org/#/c/31661/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31661/1//COMMIT_MSG@9 PS1, Line 9: not no
https://review.coreboot.org/#/c/31661/1//COMMIT_MSG@17 PS1, Line 17: include included
https://review.coreboot.org/#/c/31661/1//COMMIT_MSG@9 PS1, Line 9: Intel Braswell SoC contains SMBus controller but not support : is available for this controller. : This controller is compatible with the Intel SMBus support in the : southbridge directory. This smbus.c file in that directory : is included. : This smbus.c file can be included in build using config : SOUTHBRIDGE_INTEL_COMMON and : SOUTHBRIDGE_INTEL_COMMON_SMBUS also. This results : into build errors, caused by pmbase.c and rtc.c which is include : always.. Please add blank lines between paragraphs.
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31661
to look at the new patch set (#2).
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
soc/intel/braswell: Add SMBus support
Intel Braswell SoC contains SMBus controller but no support is available for this controller. This controller is compatible with the Intel SMBus support in the southbridge directory. This smbus.c file in that directory is included.
This smbus.c file can be includ in build using config SOUTHBRIDGE_INTEL_COMMON and SOUTHBRIDGE_INTEL_COMMON_SMBUS also. This results into build errors, caused by pmbase.c and rtc.c which is include always..
BUG=N/A TEST= Facebook FBG-1710 LCD panel
Change-Id: Ie3d4f657558a1aed21b083ef5cad08ea96e629c3 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/31661/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 2:
it seems like a better way to do it would be to edit sb/intel/common/Makefile.inc and move the conditional compilation for smbus.c outside of/above 'ifeq ($(CONFIG_SOUTHBRIDGE_INTEL_COMMON),y)' and then add 'select SOUTHBRIDGE_INTEL_COMMON_SMBUS' to Braswell's Kconfig. I tested that this builds at least
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 2:
Patch Set 2:
it seems like a better way to do it would be to edit sb/intel/common/Makefile.inc and move the conditional compilation for smbus.c outside of/above 'ifeq ($(CONFIG_SOUTHBRIDGE_INTEL_COMMON),y)' and then add 'select SOUTHBRIDGE_INTEL_COMMON_SMBUS' to Braswell's Kconfig. I tested that this builds at least
Was thinking about this solution, but unsure about the impact on boards with 'SOUTHBRIDGE_INTEL_COMMON' disabled, but 'SOUTHBRIDGE_INTEL_COMMON_SMBUS' enabled.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 2:
Was thinking about this solution, but unsure about the impact on boards with
'SOUTHBRIDGE_INTEL_COMMON' disabled, but 'SOUTHBRIDGE_INTEL_COMMON_SMBUS' enabled.
are there any such boards? surely that selection would be a noop in the present state, since smbus.c wouldn't be compiled (unless it were included as your patch currently does)
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 2:
I had a similar idea when I got a Braswell board with DIMM slot. What I did was to select SOC_INTEL_COMMON_BLOCK and SOC_INTEL_COMMON_BLOCK_SMBUS on mainboard specific options. It worked almost out of the box. There are few dependencies like soc/smbus.h file which needs: SMB_RCV_SLVA and SMBUS_SLAVE_ADDR values defined. Also, there is a need to define PCH_DEV_SMBUS elsewhere. How about that kind of approach?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 2:
which smbus implementation (sb/common or soc/common/block) is a better fit / requires less massaging to add Braswell support? that would be the one that gets my vote
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31661
to look at the new patch set (#3).
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
soc/intel/braswell: Add SMBus support
Intel Braswell SoC contains SMBus controller but no support is available for this controller. This controller is compatible with the Intel SMBus support in the southbridge common directory.
To be able using smbus support from the Intel common directory the smbus.c is moved outside SOUTHBRIDGE_INTEL_COMMON dependency block. Use SOUTHBRIDGE_INTEL_COMMON_SMBUS to include support.
BUG=N/A TEST= Facebook FBG-1710 LCD panel
Change-Id: Ie3d4f657558a1aed21b083ef5cad08ea96e629c3 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Kconfig M src/southbridge/intel/common/Makefile.inc 2 files changed, 4 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/31661/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 3: Code-Review+2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 3:
Patch Set 2:
I had a similar idea when I got a Braswell board with DIMM slot. What I did was to select SOC_INTEL_COMMON_BLOCK and SOC_INTEL_COMMON_BLOCK_SMBUS on mainboard specific options. It worked almost out of the box. There are few dependencies like soc/smbus.h file which needs: SMB_RCV_SLVA and SMBUS_SLAVE_ADDR values defined. Also, there is a need to define PCH_DEV_SMBUS elsewhere. How about that kind of approach?
The SoC common smbus only supports smbus byte read/write. The board is using I2C block write. The block functions (except i2c write) are already available in southbridge common. Therefor using SB common was the easiest.
I assume using SB common works for DIMM slot also?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
I assume using SB common works for DIMM slot also?
It should. And if not, it should be fixed.
I don't know why Intel keeps re-inventing wheels, maybe they are too bored. Usually, sb/common/ is the correct choice.
https://review.coreboot.org/#/c/31661/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31661/3//COMMIT_MSG@10 PS3, Line 10: is available for this controller. Generally no need to add a line break after every full stop.
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31661 )
Change subject: soc/intel/braswell: Add SMBus support ......................................................................
soc/intel/braswell: Add SMBus support
Intel Braswell SoC contains SMBus controller but no support is available for this controller. This controller is compatible with the Intel SMBus support in the southbridge common directory.
To be able using smbus support from the Intel common directory the smbus.c is moved outside SOUTHBRIDGE_INTEL_COMMON dependency block. Use SOUTHBRIDGE_INTEL_COMMON_SMBUS to include support.
BUG=N/A TEST= Facebook FBG-1710 LCD panel
Change-Id: Ie3d4f657558a1aed21b083ef5cad08ea96e629c3 Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31661 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/braswell/Kconfig M src/southbridge/intel/common/Makefile.inc 2 files changed, 4 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig index 061c494..fda5a6d 100644 --- a/src/soc/intel/braswell/Kconfig +++ b/src/soc/intel/braswell/Kconfig @@ -50,6 +50,7 @@ select INTEL_GMA_ACPI select INTEL_GMA_SWSMISCI select CPU_INTEL_COMMON + select SOUTHBRIDGE_INTEL_COMMON_SMBUS
config VBOOT select VBOOT_STARTS_IN_ROMSTAGE diff --git a/src/southbridge/intel/common/Makefile.inc b/src/southbridge/intel/common/Makefile.inc index 3224e1f..3ad7924 100644 --- a/src/southbridge/intel/common/Makefile.inc +++ b/src/southbridge/intel/common/Makefile.inc @@ -22,6 +22,9 @@ ramstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_RESET) += reset.c postcar-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_RESET) += reset.c
+romstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SMBUS) += smbus.c +ramstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SMBUS) += smbus.c + ifeq ($(CONFIG_SOUTHBRIDGE_INTEL_COMMON),y)
romstage-y += pmbase.c @@ -37,9 +40,6 @@ ramstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_GPIO) += gpio.c smm-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_GPIO) += gpio.c
-romstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SMBUS) += smbus.c -ramstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SMBUS) += smbus.c - romstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SPI) += spi.c postcar-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SPI) += spi.c ramstage-$(CONFIG_SOUTHBRIDGE_INTEL_COMMON_SPI) += spi.c
-
Frans Hendriks (Code Review) -
Matt DeVillier (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)