Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36211 )
Change subject: cpu/intel/car: Add EC software sync to Intel romstage
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > Patch Set 1:
> >
> > > Patch Set 1:
> > >
> > > (1 comment)
> > >
> > > > Patch Set 1:
> > > >
> > > > > Patch Set 1: Code-Review-1
> > > > >
> > > > > (3 comments)
> > > > >
> > > > > Romstage starts here for all Intel platforms. I don't think it makes sense to put platforms specific things here.
> > > >
> > > > That's fair. I'll drop this patch and look into that.
> > >
> > > The alternative is to add call points to all mainboard_romstage_entry() code which is also decoupled from the Kconfig itself. i.e. selecting Kconfig won't necessarily make things work generically.
> >
> > Don't those typically exist already to have mainboard specific init before the raminit?
>
> Somewhat. for our mainboards I try to push everyone to have a common flow instead of open coding the flow. Basically have less duplicated code. If you look back to the mainboards that had traditionally been around in coreboot they have a large amount of duplicated code. That is a maintenance burden. If Kconfig is set up correctly with the proper dependencies then I don't see an issue in placing calls at a strategic point that minimizes code duplication.
How about having a linker script 'section(.rodata.romstage_entry)' for such fucntion pointers similar to the cbmem initialization? Is that too much complexity?
--
To view, visit https://review.coreboot.org/c/coreboot/+/36211
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I31f3407a2afcbf288461fab1397f965f025bc07c
Gerrit-Change-Number: 36211
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Joel Kitching <kitching(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 23 Oct 2019 13:43:10 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36211 )
Change subject: cpu/intel/car: Add EC software sync to Intel romstage
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > Patch Set 1:
> >
> > (1 comment)
> >
> > > Patch Set 1:
> > >
> > > > Patch Set 1: Code-Review-1
> > > >
> > > > (3 comments)
> > > >
> > > > Romstage starts here for all Intel platforms. I don't think it makes sense to put platforms specific things here.
> > >
> > > That's fair. I'll drop this patch and look into that.
> >
> > The alternative is to add call points to all mainboard_romstage_entry() code which is also decoupled from the Kconfig itself. i.e. selecting Kconfig won't necessarily make things work generically.
>
> Don't those typically exist already to have mainboard specific init before the raminit?
Somewhat. for our mainboards I try to push everyone to have a common flow instead of open coding the flow. Basically have less duplicated code. If you look back to the mainboards that had traditionally been around in coreboot they have a large amount of duplicated code. That is a maintenance burden. If Kconfig is set up correctly with the proper dependencies then I don't see an issue in placing calls at a strategic point that minimizes code duplication.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36211
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I31f3407a2afcbf288461fab1397f965f025bc07c
Gerrit-Change-Number: 36211
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Joel Kitching <kitching(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 23 Oct 2019 13:36:21 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Vladimir Serbinenko,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/36229
to review the following change.
Change subject: mb/lenovo/x201/acpi: Merge common platform ASL code.
......................................................................
mb/lenovo/x201/acpi: Merge common platform ASL code.
This code in reality just describes the southbridge features, don't put a copy
in every mainboard.
This commit follows up on commit e288758b with Change-Id
I8cf3019a36b1ae6a17d502e7508f36ea9fa62830 ("bd82x6x: Merge common
platform ASL code").
Change-Id: Ic5260461165b794a13efd2c6d968c953f60dd253
Signed-off-by: Vladimir Serbinenko <phcoder(a)gmail.com>
Signed-off-by: Peter Lemenkov <lemenkov(a)gmail.com>
---
M src/mainboard/lenovo/x201/acpi/platform.asl
M src/mainboard/lenovo/x201/dsdt.asl
2 files changed, 2 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36229/1
diff --git a/src/mainboard/lenovo/x201/acpi/platform.asl b/src/mainboard/lenovo/x201/acpi/platform.asl
index 685c6ab..bcd6de6 100644
--- a/src/mainboard/lenovo/x201/acpi/platform.asl
+++ b/src/mainboard/lenovo/x201/acpi/platform.asl
@@ -14,36 +14,6 @@
* GNU General Public License for more details.
*/
-/* The APM port can be used for generating software SMIs */
-
-OperationRegion (APMP, SystemIO, 0xb2, 2)
-Field (APMP, ByteAcc, NoLock, Preserve)
-{
- APMC, 8, /* APM command */
- APMS, 8 /* APM status */
-}
-
-/* SMI I/O Trap */
-Method(TRAP, 1, Serialized)
-{
- Store (Arg0, SMIF) /* SMI Function */
- Store (0, TRP0) /* Generate trap */
- Return (SMIF) /* Return value of SMI handler */
-}
-
-/* The _PIC method is called by the OS to choose between interrupt
- * routing via the i8259 interrupt controller or the APIC.
- *
- * _PIC is called with a parameter of 0 for i8259 configuration and
- * with a parameter of 1 for Local Apic/IOAPIC configuration.
- */
-
-Method(_PIC, 1)
-{
- /* Remember the OS' IRQ routing choice. */
- Store(Arg0, PICM)
-}
-
/* The _PTS method (Prepare To Sleep) is called before the OS is
* entering a sleep state. The sleep state number is passed in Arg0
*/
diff --git a/src/mainboard/lenovo/x201/dsdt.asl b/src/mainboard/lenovo/x201/dsdt.asl
index 461892b..08e2122 100644
--- a/src/mainboard/lenovo/x201/dsdt.asl
+++ b/src/mainboard/lenovo/x201/dsdt.asl
@@ -30,6 +30,8 @@
0x20130325 /* OEM revision */
)
{
+ #include <southbridge/intel/bd82x6x/acpi/platform.asl>
+
/* Some generic macros */
#include "acpi/platform.asl"
--
To view, visit https://review.coreboot.org/c/coreboot/+/36229
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic5260461165b794a13efd2c6d968c953f60dd253
Gerrit-Change-Number: 36229
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Lemenkov <lemenkov(a)gmail.com>
Gerrit-Reviewer: Vladimir Serbinenko <phcoder(a)gmail.com>
Gerrit-MessageType: newchange
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35794 )
Change subject: mb/google/hatch/variants/helios: Modify DPTF parameters
......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35794/2/src/mainboard/google/hatch…
File src/mainboard/google/hatch/variants/helios/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/35794/2/src/mainboard/google/hatch…
PS2, Line 2: register "tdp_pl1_override" = "15"
Is this a DPTF parameter? If not, please put it into a separate commit.
--
To view, visit https://review.coreboot.org/c/coreboot/+/35794
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0e5c079856a167b1c2ef52e446d055404e565858
Gerrit-Change-Number: 35794
Gerrit-PatchSet: 5
Gerrit-Owner: Kane Chen <kane_chen(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kane Chen <kane_chen(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Philip Chen <philipchen(a)google.com>
Gerrit-Reviewer: Rajat Jain <rajatja(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Sumeet Pawnikar <sumeet.r.pawnikar(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anson Tseng <anson.tseng(a)intel.corp-partner.google.com>
Gerrit-CC: Frank Chu <frank_chu(a)pegatron.corp-partner.google.com>
Gerrit-CC: Hank Lin <hank2_lin(a)pegatron.corp-partner.google.com>
Gerrit-CC: Ken Lu <ken_lu(a)pegatron.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 23 Oct 2019 12:02:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 )
Change subject: src/soc/intel/common/itss: Add support to get IRQ configuration for PCI devices
......................................................................
Patch Set 20:
> Patch Set 20:
>
> > Patch Set 20:
> >
> > > Patch Set 20:
> > >
> > > > Patch Set 20:
> > > > Ok, But this code already creates a irq info data , I am using the same to generate the ACPI code too, I do not need to re-populate the irq matrix.
> > >
> > > This argument sounds to me as if you don't want to use the common facilities we already have because you already wrote your own reimplementation of them. I hope I misunderstand you because that's not very convincing.
> > >
> > > What is missing in acpi_pirq_gen that is present in your code (and can't be added there to avoid having all the common parts twice in our code base)?
> >
> > The intent of this code was to populate irq info based on the device tree selection and pass it to FSP for IRQ configuration based on the irq assignment rules, this implementation is added new(does not exist in acpi_pirq_gen(neither should be scoped in ACPI module)), I think
> > Arthur made a point to re-use APCI generation part from acpi_pirq_gen, to which my response was I did not want to re-populate the irq matrix again, since this code already creates that info.
>
> You don't have to repopulate anything, the matrix is a local structure of that code. You just have to provide a callback function called 'intel_common_map_pirq' which returns a route for a given pin on a given PCI device. Given your current code that should be easy or even trivial to implement. Give the FSP what it needs using this code(and please document it first!) and use the common southbridge code create ACPI _PIR tables.
>
> Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects.
>
> Please consider that reusing common code usually means avoiding pitfalls someone else fell into in the past.
Hi Arthur, I wanted to understand more on you below comment:
[Arthur]Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects.
[Aamir] I do not see multiple entries with same PCI device and int pin mapping , below is the ACPI package that is currently generated with this implementation:
Name (PIRX, Package (0x11)
{
Package (0x04)
{
0x0012FFFF,
0x00,
Zero,
0x10
},
Package (0x04)
{
0x0014FFFF,
0x00,
Zero,
0x10
},
Package (0x04)
{
0x0014FFFF,
0x01,
Zero,
0x11
},
Package (0x04)
{
0x0014FFFF,
0x02,
Zero,
0x12
},
Package (0x04)
{
0x0015FFFF,
0x00,
Zero,
0x10
},
Package (0x04)
{
0x0015FFFF,
0x01,
Zero,
0x11
},
Package (0x04)
{
0x0015FFFF,
0x02,
Zero,
0x12
},
Package (0x04)
{
0x0016FFFF,
0x00,
Zero,
0x10
},
Package (0x04)
{
0x0017FFFF,
0x00,
Zero,
0x10
},
Package (0x04)
{
0x0019FFFF,
0x03,
Zero,
0x13
},
Package (0x04)
{
0x001DFFFF,
0x00,
Zero,
0x10
},
Package (0x04)
{
0x001DFFFF,
0x01,
Zero,
0x11
},
Package (0x04)
{
0x001EFFFF,
0x00,
Zero,
0x14
},
Package (0x04)
{
0x001EFFFF,
0x01,
Zero,
0x15
},
Package (0x04)
{
0x001EFFFF,
0x02,
Zero,
0x16
},
Package (0x04)
{
0x001FFFFF,
0x00,
Zero,
0x10
},
Package (0x04)
{
0x001FFFFF,
0x01,
Zero,
0x11
}
})
And this is the IRQ table passed to FSP for configuration.(LPSS would not share same IRQs)
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/bloc…
Both aligns and do not mismatch, that is the advantage of using the same irq data for creating the APCI package as well.
--
To view, visit https://review.coreboot.org/c/coreboot/+/34089
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib7066432ff5f0d7017ac5a44922ca69f07da9556
Gerrit-Change-Number: 34089
Gerrit-PatchSet: 20
Gerrit-Owner: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Lean Sheng Tan <lean.sheng.tan(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 23 Oct 2019 09:00:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33563
Change subject: [UNTESTED]soc/intel/braswell: Use native code to update BSP microcode
......................................................................
[UNTESTED]soc/intel/braswell: Use native code to update BSP microcode
This removes the need to specify the microcode size and location in
Kconfig.
Change-Id: I9a391a3956b30dd8727bec669cb79a0a8588d5f0
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/drivers/intel/fsp1_1/cache_as_ram.S
M src/mainboard/facebook/fbg1701/Kconfig
M src/soc/intel/braswell/Kconfig
3 files changed, 10 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/33563/1
diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S
index 3460b9d..733c523 100644
--- a/src/drivers/intel/fsp1_1/cache_as_ram.S
+++ b/src/drivers/intel/fsp1_1/cache_as_ram.S
@@ -53,6 +53,15 @@
* Find the FSP binary in cbfs.
* Make a fake stack that has the return value back to this code.
*/
+
+#if CONFIG(MICROCODE_UPDATE_PRE_RAM)
+update_microcode:
+ /* put the return address in %esp */
+ movl $end_microcode_update, %esp
+ jmp update_bsp_microcode
+end_microcode_update:
+#endif
+
lea fake_fsp_stack, %esp
jmp find_fsp
find_fsp_ret:
diff --git a/src/mainboard/facebook/fbg1701/Kconfig b/src/mainboard/facebook/fbg1701/Kconfig
index b3c589d..e92022b 100644
--- a/src/mainboard/facebook/fbg1701/Kconfig
+++ b/src/mainboard/facebook/fbg1701/Kconfig
@@ -56,16 +56,6 @@
hex
default 0x00800000
-config CPU_MICROCODE_CBFS_LEN
- hex
- default 0x10C00
- help
- This should be updated when the microcode patch changes.
-
-config CPU_MICROCODE_CBFS_LOC
- hex
- default 0xFFFE9400
-
config MRC_SETTINGS_CACHE_SIZE
hex
default 0x08000
diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig
index 920179f83..45a0cf8 100644
--- a/src/soc/intel/braswell/Kconfig
+++ b/src/soc/intel/braswell/Kconfig
@@ -52,6 +52,7 @@
select CPU_INTEL_COMMON
select SOUTHBRIDGE_INTEL_COMMON_SMBUS
select C_ENVIRONMENT_BOOTBLOCK
+ select MICROCODE_UPDATE_PRE_RAM
config DCACHE_BSP_STACK_SIZE
hex
--
To view, visit https://review.coreboot.org/c/coreboot/+/33563
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9a391a3956b30dd8727bec669cb79a0a8588d5f0
Gerrit-Change-Number: 33563
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36211 )
Change subject: cpu/intel/car: Add EC software sync to Intel romstage
......................................................................
Patch Set 1:
> Patch Set 1:
>
> (1 comment)
>
> > Patch Set 1:
> >
> > > Patch Set 1: Code-Review-1
> > >
> > > (3 comments)
> > >
> > > Romstage starts here for all Intel platforms. I don't think it makes sense to put platforms specific things here.
> >
> > That's fair. I'll drop this patch and look into that.
>
> The alternative is to add call points to all mainboard_romstage_entry() code which is also decoupled from the Kconfig itself. i.e. selecting Kconfig won't necessarily make things work generically.
Don't those typically exist already to have mainboard specific init before the raminit?
--
To view, visit https://review.coreboot.org/c/coreboot/+/36211
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I31f3407a2afcbf288461fab1397f965f025bc07c
Gerrit-Change-Number: 36211
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Joel Kitching <kitching(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 23 Oct 2019 06:44:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment