coreboot-gerrit May 2017

coreboot-gerrit@coreboot.org

1 participants
3888 discussions

Change in coreboot[master]: soc/intel/skylake: Clean up code by using common FAST_SPI mo...
by Barnali Sarkar (Code Review) 02 May '17

02 May '17

Barnali Sarkar has posted comments on this change. ( https://review.coreboot.org/19055 ) Change subject: soc/intel/skylake: Clean up code by using common FAST_SPI module ...................................................................... Patch Set 22: > I believe this and the apollolake CLs will have to be rebased on > ToT. Yes Furquan, I will rebase these CLs and fix your comments and push it in some time. -- To view, visit https://review.coreboot.org/19055 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4fc90504d322db70ed4ea644b1593cc0605b5fe8 Gerrit-PatchSet: 22 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Barnali Sarkar <barnali.sarkar(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar(a)intel.com> Gerrit-Reviewer: Brandon Breitenstein <brandon.breitenstein(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc/intel/skylake: Clean up code by using common FAST_SPI mo...
by Furquan Shaikh (Code Review) 02 May '17

02 May '17

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19055 ) Change subject: soc/intel/skylake: Clean up code by using common FAST_SPI module ...................................................................... Patch Set 22: I believe this and the apollolake CLs will have to be rebased on ToT. -- To view, visit https://review.coreboot.org/19055 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4fc90504d322db70ed4ea644b1593cc0605b5fe8 Gerrit-PatchSet: 22 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Barnali Sarkar <barnali.sarkar(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar(a)intel.com> Gerrit-Reviewer: Brandon Breitenstein <brandon.breitenstein(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc/intel/skylake: Set xtal bypass on low power idle
by Furquan Shaikh (Code Review) 02 May '17

02 May '17

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19442 ) Change subject: soc/intel/skylake: Set xtal bypass on low power idle ...................................................................... Patch Set 7: Code-Review+2 -- To view, visit https://review.coreboot.org/19442 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide2d01536f652cd1b0ac32eede89ec410c5101cf Gerrit-PatchSet: 7 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Naresh Solanki <naresh.solanki(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Naresh Solanki <naresh.solanki(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Rajat Jain <rajatja(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Roy Park <roy.mingi.park(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-Reviewer: rushikesh s kadam <rushikesh.s.kadam(a)intel.com> Gerrit-HasComments: No

1 0

Change in coreboot[master]: amd/pi/hudson: Add config option for ACPI base
by Marc Jones (Code Review) 02 May '17

02 May '17

Marc Jones has submitted this change and it was merged. ( https://review.coreboot.org/19485 ) Change subject: amd/pi/hudson: Add config option for ACPI base ...................................................................... amd/pi/hudson: Add config option for ACPI base Add a configuration option to assign the binaryPI base address for the ACPI registers. The binaryPI's assignment is determine at build time and no run-time configuration is allowed. Change-Id: Ida17022abfa6faceb0653c2cb87aacce4facef09 Signed-off-by: Marc Jones <marcj303(a)gmail.com> Reviewed-on: https://review.coreboot.org/19485 Tested-by: build bot (Jenkins) Reviewed-by: Aaron Durbin <adurbin(a)chromium.org> --- M src/southbridge/amd/pi/hudson/Kconfig M src/southbridge/amd/pi/hudson/hudson.h 2 files changed, 10 insertions(+), 1 deletion(-) Approvals: Aaron Durbin: Looks good to me, approved build bot (Jenkins): Verified diff --git a/src/southbridge/amd/pi/hudson/Kconfig b/src/southbridge/amd/pi/hudson/Kconfig index 233260f..25dbdb8 100644 --- a/src/southbridge/amd/pi/hudson/Kconfig +++ b/src/southbridge/amd/pi/hudson/Kconfig @@ -219,6 +219,15 @@ help Set this option to y for serial IRQ in continuous mode. Otherwise it is in quiet mode. + +config HUDSON_ACPI_IO_BASE + hex + default 0x400 if CPU_AMD_PI_00670F00_FP4 || CPU_AMD_PI_00670F00_FT4 + default 0x800 + help + Base address for the ACPI registers. + This value must match the hardcoded value of AGESA. + endif config HUDSON_UART diff --git a/src/southbridge/amd/pi/hudson/hudson.h b/src/southbridge/amd/pi/hudson/hudson.h index e20238f..2ccd485 100644 --- a/src/southbridge/amd/pi/hudson/hudson.h +++ b/src/southbridge/amd/pi/hudson/hudson.h @@ -51,7 +51,7 @@ #define PM_YANG_SD_FLASH_CTRL 0xE8 #define PM_PCIB_CFG 0xEA -#define HUDSON_ACPI_IO_BASE 0x800 +#define HUDSON_ACPI_IO_BASE CONFIG_HUDSON_ACPI_IO_BASE #define ACPI_PM_EVT_BLK (HUDSON_ACPI_IO_BASE + 0x00) /* 4 bytes */ #define ACPI_PM1_CNT_BLK (HUDSON_ACPI_IO_BASE + 0x04) /* 2 bytes */ #define ACPI_PM_TMR_BLK (HUDSON_ACPI_IO_BASE + 0x18) /* 4 bytes */ -- To view, visit https://review.coreboot.org/19485 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ida17022abfa6faceb0653c2cb87aacce4facef09 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins)

1 0

Change in coreboot[master]: amd/pi/00670F00: Reserve A0000-FFFFF
by Marc Jones (Code Review) 02 May '17

02 May '17

Marc Jones has submitted this change and it was merged. ( https://review.coreboot.org/19156 ) Change subject: amd/pi/00670F00: Reserve A0000-FFFFF ...................................................................... amd/pi/00670F00: Reserve A0000-FFFFF Claim memory-mapped regions in the legacy area. Claim an MMIO resource for the A000 and B000 segments, and reserved resource for C000 through F000 segments. These changes allow code and information to be retained in the event unused regions get wiped. Original-Signed-off-by: Marshall Dawson <marshalldawson3rd(a)gmail.com> Reviewed-by: Marc Jones <marcj303(a)gmail.com> (cherry picked from commit d612d4fe69881609d42053496409c452e1014947) Change-Id: I9c47c919bbfd0edccf752e052f32d1e47c1a1324 Signed-off-by: Marc Jones <marcj303(a)gmail.com> Reviewed-on: https://review.coreboot.org/19156 Reviewed-by: Aaron Durbin <adurbin(a)chromium.org> Tested-by: build bot (Jenkins) --- M src/northbridge/amd/pi/00670F00/northbridge.c 1 file changed, 8 insertions(+), 0 deletions(-) Approvals: Aaron Durbin: Looks good to me, approved build bot (Jenkins): Verified diff --git a/src/northbridge/amd/pi/00670F00/northbridge.c b/src/northbridge/amd/pi/00670F00/northbridge.c index c21d030..3ba0687 100644 --- a/src/northbridge/amd/pi/00670F00/northbridge.c +++ b/src/northbridge/amd/pi/00670F00/northbridge.c @@ -850,6 +850,14 @@ assign_resources(link); } } + /* + * Reserve everything between A segment and 1MB: + * + * 0xa0000 - 0xbffff: legacy VGA + * 0xc0000 - 0xfffff: RAM + */ + mmio_resource(dev, 0xa0000, 0xa0000 / KiB, 0x20000 / KiB); + reserved_ram_resource(dev, 0xc0000, 0xc0000 / KiB, 0x40000 / KiB); } static struct device_operations pci_domain_ops = { -- To view, visit https://review.coreboot.org/19156 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I9c47c919bbfd0edccf752e052f32d1e47c1a1324 Gerrit-PatchSet: 15 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins)

1 0

Change in coreboot[master]: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge
by Marshall Dawson (Code Review) 02 May '17

02 May '17

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/19157 ) Change subject: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge ...................................................................... Patch Set 14: (9 comments) https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/ps… File src/northbridge/amd/pi/00670F00/psp.c: Line 90: static void wr_mbox_cmd_resp(struct psp_mbox *mbox, void *buffer) > Buffer is not flushed to DRAM prior to sending PSP a command to potentially That's not mentioned in the spec. I assume it's snooped like any other transaction. I think that would be a pretty bad design if not. Line 102: long usec_wait = 10000000; /* arbitrary 10 seconds */ > coverage may want 'long int' coverity? I infer it's OK. I don't see any other variables done that way, and intptr_t is typedef of long. Line 123: return PSPSTS_INIT_TIMEOUT; > At the callsite, only not-equal-to zero is tested for, and we never explici Yes. Thanks. Line 140: if (rd_mbox_sts(mbox) & STATUS_HALT) { > IMHO we should not use enum with bitwise boolean ops. Don't know if coverag > Don't know if coverage will be happy about this. I don't know. I can identify other examples, e.g. in vboot. Can you point me to more info on this? Line 150: if (wait_initialized(mbox)) { > Code should not break if we simply change order of enums, we make assumptio There are lots of examples of doing it like this, but I think it's a very reasonable change. Line 192: buffer.header.status = 0; /* PSP does not report status for this cmd */ > I can see patchset CarrizoPI uses this status field as return value from Ps It seems like send_psp_command and printk below are keeping this from getting optimized out. Then rd_resp_sts is treating it as volatile. I think it's OK here. Line 196: status_to_string(cmd_status)); > If we really get no status, no need to print it either? I kind of like seeing that the status=0. There's no string associated with that which will show up in the console (just a trailing space). https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/ps… File src/northbridge/amd/pi/00670F00/psp.h: Line 51: }; > Should we have __attribute__ ((packed)) here as well? yes Line 87: #define PSP_DEV dev_find_slot(0, PCI_DEVFN(PSP_PCI_DEV, PSP_PCI_DEV)) > PCI_DEVFN(PSP_PCI_DEV, PSP_PCI_FN) right - copy/paste fail -- To view, visit https://review.coreboot.org/19157 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf111470b261d6516b878e88be471967abe681b3 Gerrit-PatchSet: 14 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in coreboot[master]: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge
by build bot (Jenkins) (Code Review) 02 May '17

02 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19157 ) Change subject: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge ...................................................................... Patch Set 16: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-gerrit/53055/ : SUCCESS https://qa.coreboot.org/job/coreboot-checkpatch/8896/ : SUCCESS -- To view, visit https://review.coreboot.org/19157 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf111470b261d6516b878e88be471967abe681b3 Gerrit-PatchSet: 16 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in coreboot[master]: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge
by Marshall Dawson (Code Review) 02 May '17

02 May '17

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/19157 to look at the new patch set (#16). Change subject: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge ...................................................................... amd/pi/00670F00: Add AMD PSP support for Stoney Ridge Add PSP communication support that is not covered by AGESA. The PSP must be notified DRAM is ready after amdinitpost(). Move amd_initcpuio() to the end of amdinitpost() so that the PSP decode for notify works. Change-Id: Icf111470b261d6516b878e88be471967abe681b3 Signed-off-by: Marshall Dawson <marshalldawson3rd(a)gmail.com> Signed-off-by: Marc Jones <marcj303(a)gmail.com> --- M src/cpu/amd/pi/00670F00/romstage.c M src/northbridge/amd/pi/00670F00/Makefile.inc A src/northbridge/amd/pi/00670F00/psp.c A src/northbridge/amd/pi/00670F00/psp.h M src/northbridge/amd/pi/agesawrapper.c 5 files changed, 304 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/19157/16 -- To view, visit https://review.coreboot.org/19157 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icf111470b261d6516b878e88be471967abe681b3 Gerrit-PatchSet: 16 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins)

1 0

Change in coreboot[master]: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge
by build bot (Jenkins) (Code Review) 02 May '17

02 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19157 ) Change subject: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge ...................................................................... Patch Set 15: Verified-1 Build Unstable https://qa.coreboot.org/job/coreboot-gerrit/53054/ : UNSTABLE https://qa.coreboot.org/job/coreboot-checkpatch/8895/ : SUCCESS -- To view, visit https://review.coreboot.org/19157 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf111470b261d6516b878e88be471967abe681b3 Gerrit-PatchSet: 15 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in coreboot[master]: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge
by Marshall Dawson (Code Review) 02 May '17

02 May '17

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/19157 ) Change subject: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge ...................................................................... Patch Set 1: (4 comments) https://review.coreboot.org/#/c/19157/1/src/northbridge/amd/pi/00670F00/psp… File src/northbridge/amd/pi/00670F00/psp.c: Line 86: ; /* wait for initialized and no command in progress */ > ok. one more thing to fix. dont' these parts have a constant tsc? > dont' these parts have a constant tsc? Yes. Everything Family 10h and later, I believe. This has probably been propagated over the years https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/ps… File src/northbridge/amd/pi/00670F00/psp.c: Line 184 > Don't we are care about being able to know if this command failed? Short of putting it in the console log, I'm not sure what I would do with that info. Things like graphics aren't going to work properly if the notification fails or is missing. Resetting the PSP or retrying isn't in the spec, that I've found. The AMD code drops any command that fails. Line 191 > Why would we include u8 reserved[24] in the size? I think my best argument is for consistency, i.e. all other commands will also use the sizes of their mbox_xxxxxx_buffer structures. Some will have reserved bytes and others will be larger and not need padding. FWIW, the PSP seems to not check the value of .size for this particular call. Setting it to 0 before issuing the command still seemed to complete successfully. No info yet on a command that sends actual data to the PSP but is also <32 bytes. https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/ps… File src/northbridge/amd/pi/00670F00/psp.h: PS14, Line 50: mbox > If this is really a hardware register then you shouldn't be using pointer f Eww. Thanks for catching that. Wrong in AMD ref code. -- To view, visit https://review.coreboot.org/19157 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf111470b261d6516b878e88be471967abe681b3 Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2017