[coreboot-gerrit] Change in coreboot[master]: fsp_broadwell_de: Add SMM code
Werner Zeh (Code Review)
gerrit at coreboot.org
Fri Apr 7 07:26:29 CEST 2017
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/19145 )
Change subject: fsp_broadwell_de: Add SMM code
......................................................................
Patch Set 2:
(11 comments)
>How did you test this?
Tested on mc_bdx1. Following patches will add SMI functionality to mc_bdx1.
>
>Also, could maybe more code be put into a common location and shared?
I am not sure. The differences can be huge...even between Broadwell and Broadwell-DE which have nearly the same name. SMM is always highly CPU dependant and should be kept in CPU space.
https://review.coreboot.org/#/c/19145/2//COMMIT_MSG
Commit Message:
PS2, Line 13: -EMRR is now called PRMRR and the UNCORE part of it is not available
: -SMM_FEATURE_CONTROL is no longer a MSR but is now located in PCI space
: -currently only SERIRQ-SMI has a handler
> Please add a space after the “bullet point” (-).
Sure.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/cpu.c
File src/soc/intel/fsp_broadwell_de/cpu.c:
PS2, Line 70: /* Now that all APs have been relocated as well as the BSP let SMIs
: * start flowing. */
> use correct style for multi-line comment
Sure, will update all comments in the touched files to the new style.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/include/soc/broadwell_de.h
File src/soc/intel/fsp_broadwell_de/include/soc/broadwell_de.h:
Line 29:
> One blank line should be enough?
Yes, thanks Paul.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/include/soc/msr.h
File src/soc/intel/fsp_broadwell_de/include/soc/msr.h:
PS2, Line 37: #define SMRR_SUPPORTED (1<<11)
: #define PRMRR_SUPPORTED (1<<12)
> Please add spaces around the shift operator.
Will do.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/include/soc/pci_devs.h
File src/soc/intel/fsp_broadwell_de/include/soc/pci_devs.h:
Line 135:
> One blank line should be enough.
Will do.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
File src/soc/intel/fsp_broadwell_de/include/soc/smm.h:
PS2, Line 38: /* The smm_save_state_in_msrs field indicates if SMM save state
: * locations live in MSRs. This indicates to the CPUs how to adjust
: * the SMMBASE and IEDBASE */
> I believe it was decided on the style where in non-first lines no asterisk
Yes.
PS2, Line 44: /* There is a bug in the order of Kconfig includes in that arch/x86/Kconfig
: * is included after chipset code. This causes the chipset's Kconfig to be
: * clobbered by the arch/x86/Kconfig if they have the same name. */
: static inline int smm_region_size(void)
: {
: /* Make it 8MiB by default. */
: if (CONFIG_SMM_TSEG_SIZE == 0)
: return (8 << 20);
: return CONFIG_SMM_TSEG_SIZE;
: }
:
: void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
: uintptr_t staggered_smbase);
: void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
: size_t *smm_save_state_size);
: void smm_initialize(void);
: void smm_relocate(void);
:
: /* These helpers are for performing SMM relocation. */
: void southbridge_trigger_smi(void);
: void southbridge_clear_smi_status(void);
:
: /* The initialization of the southbridge is split into 2 components. One is
: * for clearing the state in the SMM registers. The other is for enabling
: * SMIs. They are split so that other work between the 2 actions. */
> Please use the elaborate commenting style, as it’s outside of a function.
Will do.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/pmutil.c
File src/soc/intel/fsp_broadwell_de/pmutil.c:
PS2, Line 49: /*
: * PM1_CNT
: */
> is this needed? also PM1 and SMI below
The comment itself is not needed, as the function names provides the same information. I will delete the comments.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/smi.c
File src/soc/intel/fsp_broadwell_de/smi.c:
PS2, Line 73: * Using the local apic is a bit more tricky. According to
: * AMD Family 11 Processor BKDG no destination shorthand must be
: * used.
: * The whole SMM initialization is quite a bit hardware specific, so
: * I'm not too worried about the better of the methods at the moment
: */
> Should the AMD reference be removed?
I am not sure. This comment just tells the reader that the injection of an SMI is hardware dependent and the mention of AMD provides information for a different hardware and therefore points out the differences better.
Do you persist on removing it from this file?
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/smihandler.c
File src/soc/intel/fsp_broadwell_de/smihandler.c:
PS2, Line 96: /* We need to clear the SMI status registers, or we won't see what's
: * happening in the following calls.
: */
> I think the style should be changed.
Yes, will do.
https://review.coreboot.org/#/c/19145/2/src/soc/intel/fsp_broadwell_de/smmrelocate.c
File src/soc/intel/fsp_broadwell_de/smmrelocate.c:
PS2, Line 59: /* The relocated handler runs with all CPUs concurrently. Therefore
: * stagger the entry points adjusting SMBASE downwards by save state
: * size * CPU num. */
> multi-line comment style anywhere in this file
Will do.
--
To view, visit https://review.coreboot.org/19145
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I461a14d411aedefdb0cb54ae43b91103a80a4f6a
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Mario Scheithauer <mario.scheithauer at siemens.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Reviewer: York Yang <york.yang at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes
More information about the coreboot-gerrit
mailing list