Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27369 )
Change subject: soc/intel/basecode: Add support for updating ucode loaded via FIT
......................................................................
Patch Set 69:
(4 comments)
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/base…
File src/soc/intel/common/basecode/fw_update/ucode_update.c:
https://review.coreboot.org/c/coreboot/+/27369/67/src/soc/intel/common/base…
PS67, Line 260: BOOT_STATE_INIT_ENTRY(BS_PRE_DEVICE, BS_ON_ENTRY, ucode_fw_sync, NULL);
> > This will be moved as part of intel_fw_update where coreboot triggers CSE FW Update and UCODE Upda […]
Ack
https://review.coreboot.org/c/coreboot/+/27369/69/src/soc/intel/common/base…
File src/soc/intel/common/basecode/fw_update/ucode_update.c:
https://review.coreboot.org/c/coreboot/+/27369/69/src/soc/intel/common/base…
PS69, Line 58: halt();
: }
> die already calls halt()
Done
https://review.coreboot.org/c/coreboot/+/27369/69/src/soc/intel/common/base…
PS69, Line 131: goto failure;
> There is only one occurrence of this so remove the goto?
Done
https://review.coreboot.org/c/coreboot/+/27369/69/src/soc/intel/common/base…
PS69, Line 249: void ucode_fw_sync(void *unused)
> any reason to not make this static?
This is going to be called from intel_fw_update
https://review.coreboot.org/c/coreboot/+/46819/6/src/soc/intel/common/basec…
I will remove and make it static for now. I will make is global with the corresponding patch.
--
To view, visit https://review.coreboot.org/c/coreboot/+/27369
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iab6ba36a2eb587f331fe522c778e2c430c8eb655
Gerrit-Change-Number: 27369
Gerrit-PatchSet: 69
Gerrit-Owner: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
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: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma(a)intel.com>
Gerrit-CC: Dhaval Sharma <dhaval.v.sharma(a)intel.corp-partner.google.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-CC: rushikesh s kadam <rushikesh.s.kadam(a)intel.com>
Gerrit-Comment-Date: Thu, 10 Dec 2020 03:14:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Sugnan Prabhu S has uploaded a new patch set (#70) to the change originally created by Rizwan Qureshi. ( https://review.coreboot.org/c/coreboot/+/27369 )
Change subject: soc/intel/basecode: Add support for updating ucode loaded via FIT
......................................................................
soc/intel/basecode: Add support for updating ucode loaded via FIT
Intel’s FIT (Firmware Interface Table) based MCU (microcode/pcode patch)
loading mechanism patches the microcode before CPU reset. In the current
Chromebooks, field updatable FW has to be first verified by vboot. Since
the MCU is loaded before reset, vboot cannot verify the same and hence
we end up restricting FIT based MCU update only from RO.
This patch implements a scheme which will allow chromebooks to update
MCU in the field.
Create 2 bootblocks (use INTEL_ADD_TOP_SWAP_BOOTBLOCK) each containing
their own FIT table. First bootblock FIT has pointers to MCUs
(in microcode_blob.bin) which resides in RO section. This will be used
in the recovery scenario and also when booting with top-swap disabled
i.e, RTC reset. Second bootblock (Normal mode) is identical to the first
one except the FIT. Insert an additional pointer to a MCU that will
reside in a staging area. Use the
CONFIG_INTEL_TOP_SWAP_FIT_ENTRY_FMAP_REG config to insert the address
of the staging area into FIT.
Top swap control bit in RTC (BUC) PCR register (0x3414) is used to
switch between the two bootblocks.
Reserve a region in the FMAP which is equal to or greater than the MCU
size specified in the BWG for a particular SoC (e.g., for Skylake/Kaby
Lake it is 192K). This is a RW region just like the RW_MRC_CACHE. MCU
from RW-A/RW-B will be copied to this region during boot. Protect this
staging area with a FPR.
Basic update flow:
In non-recovery mode, Once a slot has been selected and loaded, check if
the current slot MCU and RW staging MCU are same. If not, update the
staging area with the MCU found in the current slot and reset the
system.
Also, make sure that the top-swap is enabled in normal/developer mode
and disabled in recovery mode.
In order to enable the update feature:
* The mainboard chromeos.fmd should include a new region for staging MCU
e.g, RW_UCODE_STAGED.
* Select config INTEL_TOP_SWAP_MULTI_FIT_UCODE_UPDATE.
Add documentation to describing the MCU update procedure.
Update config name and Makefile.inc
TODO: Since this update mechanism is dealing mostly with a single MCU
it is best suited for systems where the CPU is soldered down and not
replaceable (socketed). Extend the update mechanism to systems where the
CPU is replaceable, by including multiple MCU for different CPUs.
TEST=Create an FW image for soraka and flash, create a
chromeos-firmwareupdate shellball with a newer MCU and perform an
update. Make sure that the currently loaded microcode version
matches the one in firmwareupdate.
Change-Id: Iab6ba36a2eb587f331fe522c778e2c430c8eb655
Signed-off-by: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Signed-off-by: dhaval v sharma <dhaval.v.sharma(a)intel.com>
Signed-off-by: Pandya, Varshit B <varshit.b.pandya(a)intel.com>
Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
---
M Documentation/soc/intel/index.md
A Documentation/soc/intel/ucode_update/flash_layout.svg
A Documentation/soc/intel/ucode_update/microcode_update_model.md
M Makefile.inc
A src/soc/intel/common/basecode/fw_update/Kconfig
A src/soc/intel/common/basecode/fw_update/Makefile.inc
A src/soc/intel/common/basecode/fw_update/ucode_update.c
7 files changed, 667 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/27369/70
--
To view, visit https://review.coreboot.org/c/coreboot/+/27369
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iab6ba36a2eb587f331fe522c778e2c430c8eb655
Gerrit-Change-Number: 27369
Gerrit-PatchSet: 70
Gerrit-Owner: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
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: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma(a)intel.com>
Gerrit-CC: Dhaval Sharma <dhaval.v.sharma(a)intel.corp-partner.google.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-CC: rushikesh s kadam <rushikesh.s.kadam(a)intel.com>
Gerrit-MessageType: newpatchset
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48385 )
Change subject: soc/intel/common/block/uart: rework to use dummy device
......................................................................
Patch Set 3: Code-Review-1
--
To view, visit https://review.coreboot.org/c/coreboot/+/48385
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic9c9398829b52e6b0523504b862aae9aff559bc7
Gerrit-Change-Number: 48385
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 10 Dec 2020 02:53:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48385 )
Change subject: soc/intel/common/block/uart: rework to use dummy device
......................................................................
Patch Set 3:
> Patch Set 3:
>
> I only now realized that you stripped the most important
> part when quoting me:
>
> > > > > What
> > > > > about all the mainboards that use this driver by properly
> > > > > matching the PCI IDs?
>
> Maybe you misunderstood. This driver is not only used
> via a `chip` entry in the devicetree. It is foremost
> used as a usual PCI driver.
>
> $ git grep 'device pci 1[89e].* on.*UART' src/mainboard/
>
> This says there are at least 120 cases in the tree that
> potentially use it and might break due to this change. The
> driver would even be used if the device is not mentioned
> in the dt at all, it's PCI.
Ok, that's what *I* missed then...
>
> > > > > I can't say if this is working, or even if it can work.
> > > >
> > > > I wonder how this one is different from drivers/wifi/generic, which uses the same dummy model?
> > >
> > > Except it doesn't? It does not use a generic device below
> > > a PCI device. And it checks the device path so it doesn't
> > > attach PCI ops to a non-PCI device in the first place.
> >
> > What is this then?
> >
> > device pci 14.3 on
> > chip drivers/wifi/generic
> > register "wake" = "GPE0_PME_B0"
> > device generic 0 on end
> > end
> > end # CNVi wifi
>
> Sorry, my bad. I didn't realize it's used with two
> different topologies. The difference is that the driver
> is prepared for that and attaches the CNVi ops only
> in case of the generic device. And only then the parent
> is queried.
>
> Here, OTOH, you always use the parent, no matter the
> topology.
Ack, got it. Thanks
>
> >
> > You really should check the history of drivers/wifi/generic, commit d436750 for example...
>
> Well, the history is one thing. The current code is what
> matters however. How about you read that?
I did ;) Actually I meant that one change, the wording is just misleading
Well, if we rework the driver/chip modeling like you proposed, we indeed can get rid of any fake devices. Also I'm not sure if we want to mix "drivers" and "chips" then, because it's not always certain which model needs to be used. Or would we keep chips for real devices then and only use "driver" when the port is the device?
Where would we place driver options then?
> device pci 19.2 hidden
> driver soc/intel/common/block/uart
> end
>
> device pci 14.3 on
> driver drivers/wifi/generic
> register "wake" = "GPE0_PME_B0"
> end # CNVi wifi
--
To view, visit https://review.coreboot.org/c/coreboot/+/48385
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic9c9398829b52e6b0523504b862aae9aff559bc7
Gerrit-Change-Number: 48385
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 10 Dec 2020 02:53:14 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment