Christian Gmeiner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31461
Change subject: intel/apollolake: Add early smbus support ......................................................................
intel/apollolake: Add early smbus support
Change-Id: Ic472c71998064d09c9caddc5c80c01e85a381c69 Signed-off-by: Christian Gmeiner christian.gmeiner@gmail.com --- M src/soc/intel/apollolake/Kconfig M src/soc/intel/apollolake/bootblock/bootblock.c M src/soc/intel/apollolake/include/soc/smbus.h 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/31461/1
diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig index 19cd296..f7ddc6f 100644 --- a/src/soc/intel/apollolake/Kconfig +++ b/src/soc/intel/apollolake/Kconfig @@ -85,6 +85,7 @@ select SOC_INTEL_COMMON_BLOCK_PCR select SOC_INTEL_COMMON_BLOCK_P2SB select SOC_INTEL_COMMON_BLOCK_PMC + select SOC_INTEL_COMMON_BLOCK_SMBUS select SOC_INTEL_COMMON_BLOCK_SRAM select SOC_INTEL_COMMON_BLOCK_RTC select SOC_INTEL_COMMON_BLOCK_SA diff --git a/src/soc/intel/apollolake/bootblock/bootblock.c b/src/soc/intel/apollolake/bootblock/bootblock.c index cf3e839..99a9467 100644 --- a/src/soc/intel/apollolake/bootblock/bootblock.c +++ b/src/soc/intel/apollolake/bootblock/bootblock.c @@ -23,6 +23,7 @@ #include <intelblocks/p2sb.h> #include <intelblocks/pcr.h> #include <intelblocks/rtc.h> +#include <intelblocks/smbus.h> #include <intelblocks/systemagent.h> #include <intelblocks/pmclib.h> #include <intelblocks/tco.h> @@ -112,6 +113,9 @@ /* Program TCO Timer Halt */ tco_configure();
+ /* Program SMBUS_BASE_ADDRESS and Enable it */ + smbus_common_init(); + /* Use Nx and paging to prevent the frontend from writing back dirty * cache-as-ram lines to backing store that doesn't exist when the L1I * speculatively fetches a line that is sitting in the L1D. */ diff --git a/src/soc/intel/apollolake/include/soc/smbus.h b/src/soc/intel/apollolake/include/soc/smbus.h index 4b252d6..a4ff451 100644 --- a/src/soc/intel/apollolake/include/soc/smbus.h +++ b/src/soc/intel/apollolake/include/soc/smbus.h @@ -16,6 +16,9 @@ #ifndef _SOC_APOLLOLAKE_SMBUS_H_ #define _SOC_APOLLOLAKE_SMBUS_H_
+/* PCI Configuration Space (D31:F3): SMBus */ +#define SMB_RCV_SLVA 0x09 + /* TCO registers and fields live behind TCOBASE I/O bar in SMBus device. */ #define TCO1_STS 0x04 #define TCO_TIMEOUT (1 << 3) @@ -25,4 +28,7 @@ #define TCO_LOCK (1 << 12) #define TCO_TMR_HLT (1 << 11)
+/* SMBus I/O bits. */ +#define SMBUS_SLAVE_ADDR 0x24 + #endif
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add early smbus support ......................................................................
Patch Set 1: Code-Review+1
Tested how?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add early smbus support ......................................................................
Patch Set 1:
What do you need it for?
NB. I started with something similar (but in romstage) for my APL port, but realized that FSP wants to read SPDs by itself anyway.
Christian Gmeiner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add early smbus support ......................................................................
Patch Set 1:
Patch Set 1:
What do you need it for?
NB. I started with something similar (but in romstage) for my APL port, but realized that FSP wants to read SPDs by itself anyway.
On the target board there is an EEPROM at 0xa0 but it does _NOT_ contain SPD data but a custom data structure used by our hardware provider to store memory configuration stuff.
During romstage I need to read out the EEPROM and fill FSP memory structure.
Board support patch should hit gerrit soon.
Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31461
to look at the new patch set (#2).
Change subject: intel/apollolake: Add early smbus support ......................................................................
intel/apollolake: Add early smbus support
Change-Id: Ic472c71998064d09c9caddc5c80c01e85a381c69 Signed-off-by: Christian Gmeiner christian.gmeiner@gmail.com --- M src/soc/intel/apollolake/Kconfig M src/soc/intel/apollolake/include/soc/smbus.h M src/soc/intel/apollolake/romstage.c 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/31461/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add early smbus support ......................................................................
Patch Set 2: Code-Review-1
Not all boards in tree use the respective pads for SMBus. Might be better to call this from the mainboard level.
Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31461
to look at the new patch set (#3).
Change subject: intel/apollolake: Add smbus defines ......................................................................
intel/apollolake: Add smbus defines
These defines are needed to use the smbus in mainboard code.
Change-Id: Ic472c71998064d09c9caddc5c80c01e85a381c69 Signed-off-by: Christian Gmeiner christian.gmeiner@gmail.com --- M src/soc/intel/apollolake/include/soc/smbus.h 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/31461/3
Christian Gmeiner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
gentle ping
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
gentle ping
I have no idea how the defines could be useful for mainboard code; and it seems there is no push of the latter to look at how they are used?
Generally I wonder, why the mainboard code would have to poke registers. We have APIs for device access (maybe something is missing for APL, though).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
I'd like to see the follow-ups too. From what it looks like, you would want to implement the SMBus slave mode? Why? Is there BMC or a NIC that acts as a second SMBus master on the same bus?
Christian Gmeiner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
Patch Set 3:
gentle ping
I have no idea how the defines could be useful for mainboard code; and it seems there is no push of the latter to look at how they are used?
Generally I wonder, why the mainboard code would have to poke registers. We have APIs for device access (maybe something is missing for APL, though).
In my mainboard KConfig I have this now:
config BOARD_SPECIFIC_OPTIONS def_bool y select SOC_INTEL_APOLLOLAKE select SOC_INTEL_COMMON_BLOCK_SMBUS <--- ..
As it was not accepted to select SOC_INTEL_COMMON_BLOCK_SMBUS in generic APL code I need to do it in my mainboard code cause of GPIO settings. And this ends in this compile error.
CC ramstage/soc/intel/common/block/smm/smm.o CC ramstage/soc/intel/common/block/smbus/smbus.o src/soc/intel/common/block/smbus/smbus.c: In function 'pch_smbus_init': src/soc/intel/common/block/smbus/smbus.c:65:8: error: 'SMBUS_SLAVE_ADDR' undeclared (first use in this function); did you mean 'SMBUS_ERROR'? outb(SMBUS_SLAVE_ADDR, res->base + SMB_RCV_SLVA); ^~~~~~~~~~~~~~~~ SMBUS_ERROR src/soc/intel/common/block/smbus/smbus.c:65:8: note: each undeclared identifier is reported only once for each function it appears in src/soc/intel/common/block/smbus/smbus.c:65:38: error: 'SMB_RCV_SLVA' undeclared (first use in this function) outb(SMBUS_SLAVE_ADDR, res->base + SMB_RCV_SLVA); ^~~~~~~~~~~~ make: *** [Makefile:356: build/ramstage/soc/intel/common/block/smbus/smbus.o] Fehler 1
Do may ask why I need this at all - I need to readout the params used to init the memory from an eeprom connected via smbus that does _NOT_ contain SPD.
void mainboard_memory_init_params(FSPM_UPD *mupd) { uint8_t buf[0x100] = { 0 }; int i;
/* setup early gpio before memory */ gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table));
for (i = 0; i < ARRAY_SIZE(buf); i++) buf[i] = smbus_read_byte(0, 0x50, i);
struct memory_config *m = (struct memory_config *) buf; mupd->FspmConfig.Package = m->package; mupd->FspmConfig.Profile = m->profile; mupd->FspmConfig.MemoryDown = m->memory_down; mupd->FspmConfig.DDR3LPageSize = m->DDR3LPageSize; mupd->FspmConfig.DDR3LASR = m->DDR3LASR; mupd->FspmConfig.ScramblerSupport = m->scrambler_support; mupd->FspmConfig.InterleavedMode = m->interleaved_mode; mupd->FspmConfig.ChannelHashMask = m->channel_hash_mask; mupd->FspmConfig.SliceHashMask = m->slice_hash_mask; mupd->FspmConfig.ChannelsSlicesEnable = m->channels_slices_enable; mupd->FspmConfig.MinRefRate2xEnable = m->min_ref_rate_2x_enable; mupd->FspmConfig.DualRankSupportEnable = m->dual_rank_support_enable; mupd->FspmConfig.RmtMode = m->rmt_mode; mupd->FspmConfig.MemorySizeLimit = m->memory_size_limit; mupd->FspmConfig.LowMemoryMaxValue = m->low_memory_max_value; mupd->FspmConfig.HighMemoryMaxValue = m->high_memory_max_value; mupd->FspmConfig.DisableFastBoot = m->disable_fast_boot; mupd->FspmConfig.DIMM0SPDAddress = m->spd_address_0; mupd->FspmConfig.DIMM1SPDAddress = m->spd_address_1; mupd->FspmConfig.Ch0_RankEnable = m->ch0_rank_enable; mupd->FspmConfig.Ch0_DeviceWidth = m->ch0_device_width; mupd->FspmConfig.Ch0_DramDensity = m->ch0_dram_density; mupd->FspmConfig.Ch0_Option = m->ch0_option; mupd->FspmConfig.Ch0_OdtConfig = m->ch0_odt_config; mupd->FspmConfig.Ch0_TristateClk1 = m->ch0_tristateClk1; mupd->FspmConfig.Ch0_Mode2N = m->ch0_Mode2N; mupd->FspmConfig.Ch0_OdtLevels = m->ch0_OdtLevels; mupd->FspmConfig.Ch1_RankEnable = m->ch1_rank_enable; mupd->FspmConfig.Ch1_DeviceWidth = m->ch1_device_width; mupd->FspmConfig.Ch1_DramDensity = m->ch1_dram_density; mupd->FspmConfig.Ch1_Option = m->ch1_option; mupd->FspmConfig.Ch1_OdtConfig = m->ch1_OdtLevels; mupd->FspmConfig.Ch1_TristateClk1 = m->ch1_tristateClk1; mupd->FspmConfig.Ch1_Mode2N = m->ch1_Mode2N; mupd->FspmConfig.Ch1_OdtLevels = m->ch1_OdtLevels; mupd->FspmConfig.Ch2_RankEnable = m->ch2_rank_enable; mupd->FspmConfig.Ch2_DeviceWidth = m->ch2_device_width; mupd->FspmConfig.Ch2_DramDensity = m->ch2_dram_density; mupd->FspmConfig.Ch2_Option = m->ch2_option; mupd->FspmConfig.Ch2_OdtConfig = m->ch2_odt_config; mupd->FspmConfig.Ch2_TristateClk1 = m->ch2_tristateClk1; mupd->FspmConfig.Ch2_Mode2N = m->ch2_Mode2N; mupd->FspmConfig.Ch2_OdtLevels = m->ch2_OdtLevels; mupd->FspmConfig.Ch3_RankEnable = m->ch3_rank_enable; mupd->FspmConfig.Ch3_DeviceWidth = m->ch3_device_width; mupd->FspmConfig.Ch3_DramDensity = m->ch3_dram_density; mupd->FspmConfig.Ch3_Option = m->ch3_option; mupd->FspmConfig.Ch3_OdtConfig = m->ch3_odt_config; mupd->FspmConfig.Ch3_TristateClk1 = m->ch3_tristateClk1; mupd->FspmConfig.Ch3_Mode2N = m->ch3_Mode2N; mupd->FspmConfig.Ch3_OdtLevels = m->ch3_OdtLevels; mupd->FspmConfig.RmtCheckRun = m->rmt_check_run; mupd->FspmConfig.RmtMarginCheckScaleHighThreshold = m->rmt_margin_chk_scale_hi_threshold;
memcpy(mupd->FspmConfig.Ch0_Bit_swizzling, m->ch0_bitSwizzling, sizeof(m->ch0_bitSwizzling)); memcpy(mupd->FspmConfig.Ch1_Bit_swizzling, m->ch1_bitSwizzling, sizeof(m->ch1_bitSwizzling)); memcpy(mupd->FspmConfig.Ch2_Bit_swizzling, m->ch2_bitSwizzling, sizeof(m->ch2_bitSwizzling)); memcpy(mupd->FspmConfig.Ch3_Bit_swizzling, m->ch3_bitSwizzling, sizeof(m->ch3_bitSwizzling)); }
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
gentle ping
I have no idea how the defines could be useful for mainboard code; and it seems there is no push of the latter to look at how they are used?
Generally I wonder, why the mainboard code would have to poke registers. We have APIs for device access (maybe something is missing for APL, though).
In my mainboard KConfig I have this now:
config BOARD_SPECIFIC_OPTIONS def_bool y select SOC_INTEL_APOLLOLAKE select SOC_INTEL_COMMON_BLOCK_SMBUS <--- ..
As it was not accepted to select SOC_INTEL_COMMON_BLOCK_SMBUS in generic APL code I need to do it in my mainboard code cause of GPIO settings. And this ends in this compile error.
This might be a misunderstanding. A `select SOC_INTEL_ COMMON_BLOCK_SMBUS` always belongs into the soc/ code. It reflects that this SoC supports the common SMBus implementation. What I asked to move into the mainboard code was the early call to initialize it in romstage.
Also, when we ask for the mainboard code: in the end it should be a reviewed patch on Gerrit, before we merge compilation fixes that wouldn't exist without it. Simply because there is no reason to fix something upstream that is not build tested upstream.
Christian Gmeiner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
Patch Set 3:
gentle ping
I have no idea how the defines could be useful for mainboard code; and it seems there is no push of the latter to look at how they are used?
Generally I wonder, why the mainboard code would have to poke registers. We have APIs for device access (maybe something is missing for APL, though).
In my mainboard KConfig I have this now:
config BOARD_SPECIFIC_OPTIONS def_bool y select SOC_INTEL_APOLLOLAKE select SOC_INTEL_COMMON_BLOCK_SMBUS <--- ..
As it was not accepted to select SOC_INTEL_COMMON_BLOCK_SMBUS in generic APL code I need to do it in my mainboard code cause of GPIO settings. And this ends in this compile error.
This might be a misunderstanding. A `select SOC_INTEL_ COMMON_BLOCK_SMBUS` always belongs into the soc/ code. It reflects that this SoC supports the common SMBus implementation. What I asked to move into the mainboard code was the early call to initialize it in romstage.
Ahh.. got it.
Also, when we ask for the mainboard code: in the end it should be a reviewed patch on Gerrit, before we merge compilation fixes that wouldn't exist without it. Simply because there is no reason to fix something upstream that is not build tested upstream.
I will push the mainboard too (after I double checked the devicetree.cb)
Thanks
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31461/3/src/soc/intel/apollolake/include/soc... File src/soc/intel/apollolake/include/soc/smbus.h:
https://review.coreboot.org/#/c/31461/3/src/soc/intel/apollolake/include/soc... PS3, Line 20: #define SMB_RCV_SLVA 0x09 Please use tabs for alignment as below.
https://review.coreboot.org/#/c/31461/3/src/soc/intel/apollolake/include/soc... PS3, Line 32: #define SMBUS_SLAVE_ADDR 0x24 As above.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31461 )
Change subject: intel/apollolake: Add smbus defines ......................................................................
Patch Set 3:
I'd say this is obsolete by now.
-
Christian Gmeiner (Code Review) -
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)