coreboot-gerrit November 2019

coreboot-gerrit@coreboot.org

1 participants
2418 discussions

Change in coreboot[master]: include: introduce update* for mmio operations
by Michael Niewöhner (Code Review) 04 Nov '19

04 Nov '19

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36596 ) Change subject: include: introduce update* for mmio operations ...................................................................... Uploaded patch set 2. (2 comments) https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h File src/include/mmio.h: https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h@19 PS1, Line 19: static __always_inline void update8(const volatile void *addr, uint8_t mask, uint8_t or) > put 'mmio' in the name so it's clear. yell, read* and write* don't have mmio in it's name. shouldn't we be consistent? https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h@21 PS1, Line 21: read8 > #include what you use. looks like #include <device/mmio.h> would work. oops, ofc. thx -- To view, visit https://review.coreboot.org/c/coreboot/+/36596 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f188d586c09ee9f1e17290563f68970603204fb Gerrit-Change-Number: 36596 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 Nov 2019 17:50:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: include: introduce update* for mmio operations
by Michael Niewöhner (Code Review) 04 Nov '19

04 Nov '19

Hello Aaron Durbin, Arthur Heymans, build bot (Jenkins), Nico Huber, Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/36596 to look at the new patch set (#2). Change subject: include: introduce update* for mmio operations ...................................................................... include: introduce update* for mmio operations Introduce update* as equivalent of pci_update* for mmio operations. Change-Id: I9f188d586c09ee9f1e17290563f68970603204fb Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de> --- A src/include/mmio.h 1 file changed, 41 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/36596/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/36596 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f188d586c09ee9f1e17290563f68970603204fb Gerrit-Change-Number: 36596 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: include: introduce update* for mmio operations
by Aaron Durbin (Code Review) 04 Nov '19

04 Nov '19

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36596 ) Change subject: include: introduce update* for mmio operations ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h File src/include/mmio.h: https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h@19 PS1, Line 19: static __always_inline void update8(const volatile void *addr, uint8_t mask, uint8_t or) put 'mmio' in the name so it's clear. https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h@21 PS1, Line 21: read8 #include what you use. looks like #include <device/mmio.h> would work. -- To view, visit https://review.coreboot.org/c/coreboot/+/36596 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f188d586c09ee9f1e17290563f68970603204fb Gerrit-Change-Number: 36596 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 Nov 2019 17:24:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: pmclib: make use of the new ETR address API
by Aaron Durbin (Code Review) 04 Nov '19

04 Nov '19

1 0

Change in coreboot[master]: soc/intel/cannonlake: lockdown: lock global reset
by Aaron Durbin (Code Review) 04 Nov '19

04 Nov '19

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36571 ) Change subject: soc/intel/cannonlake: lockdown: lock global reset ...................................................................... Patch Set 4: Since the semantics of pmc_global_reset_lock() changed, then apollolake/chip.c should change as well. -- To view, visit https://review.coreboot.org/c/coreboot/+/36571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6aba9bcb2ad09e6ae0e02d8c0b552e34bdb3fa72 Gerrit-Change-Number: 36571 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 Nov 2019 16:44:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: pmclib: make use of the new ETR address API
by Aaron Durbin (Code Review) 04 Nov '19

04 Nov '19

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36570 ) Change subject: soc/intel/common: pmclib: make use of the new ETR address API ...................................................................... Patch Set 4: (2 comments) https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block… File src/soc/intel/common/block/pmc/pmclib.c: https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block… PS4, Line 438: reg = (reg & ~CF9_GLB_RST) | CF9_LOCK; This is still changing the semantics of the previous implementation. apl was doing the following: static void soc_final(void *data) { >-------/* Disable global reset, just in case */ >-------pmc_global_reset_enable(0); >-------/* Make sure payload/OS can't trigger global reset */ >-------pmc_global_reset_lock(); } Now, if we want to combine that into a single function we certainly can. However, I think you should name it accordingly. i.e. you are also clearing global reset request flag as well. https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block… PS4, Line 432: void pmc_global_reset_lock(void) : { : uint32_t *etr = soc_read_pmc_etr_addr(); : uint32_t reg; : : reg = read32(etr); : reg = (reg & ~CF9_GLB_RST) | CF9_LOCK; : write32(etr, reg); : } > with CB:36596 the mmio part would translate to this: […] How was one going to convey the difference of the location of the register? Add another Kconfig? It still needs: 1. if mmconf need the device and then implicitly use the name soc/pci_devs.h 2. Need definition of ETR offset which chagnes by chipset which is being provided through soc/pm.h. You can add a Kconfig and then provide a helper function in here to provide the address or ETR. It'd still be a "callback" in the sense one shouldn't be sprinkling the logic of determining those preconditions all over the code. -- To view, visit https://review.coreboot.org/c/coreboot/+/36570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49f59efb4a7c7d3d629ac54a7922bbcc8a87714d Gerrit-Change-Number: 36570 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 Nov 2019 16:43:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: Michael Niewöhner Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake: add soc implementation for ETR address API
by Aaron Durbin (Code Review) 04 Nov '19

04 Nov '19

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36569 ) Change subject: soc/intel/skylake: add soc implementation for ETR address API ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmut… File src/soc/intel/skylake/pmutil.c: https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmut… PS4, Line 178: pcicfg(PCH_DEVFN_PMC << 12)->reg32[ETR / sizeof(uint32_t)]; : } > this was requested by Aaron pci ops don't provide an address. If one wanted to not expose some of the PCI hairiness in our code base then one would need to provide read()/write() ETR API. We can clean this up some by providing equivalent helper functions in pci_mmio_cfg.h and utilizing pci_ops.h. However, I will note we know for a fact that skl has mmconfig and utilizes it so it's not like we need to handle io vs mmio pci config access. In this function we'd have to do: return pci_mmio_config32_addr(pcidev_bdf(PCH_DEV_PMC), ETR); That would inherently do a pci device lookup in the pci device tree and then encode the pci device path into what is needed to match with pci_devfn_t. And then we'd get take the address of the register. Now, there's plans to remove the duplication of encoding. I have some patches I'm working on as well as Nico and Kyosti. This will likely clean itself up sooner than later. diff --git a/src/include/device/pci_mmio_cfg.h b/src/include/device/pci_mmio_cfg.h index 5567ed86ae..6e6668e11c 100644 --- a/src/include/device/pci_mmio_cfg.h +++ b/src/include/device/pci_mmio_cfg.h @@ -86,6 +86,24 @@ void pci_mmio_write_config32(pci_devfn_t dev, uint16_t reg, uint32_t value) pcicfg(dev)->reg32[reg / sizeof(uint32_t)] = value; } +static __always_inline +uint8_t *pci_mmio_config8_addr(pci_devfn_t dev, uint16_t reg) +{ + return &pcicfg(dev)->reg8[reg]; +} + +static __always_inline +uint16_t *pci_mmio_config16_addr(pci_devfn_t dev, uint16_t reg) +{ + return &pcicfg(dev)->reg16[reg / sizeof(uint16_t)]; +} + +static __always_inline +uint32_t *pci_mmio_config32_addr(pci_devfn_t dev, uint16_t reg) +{ + return &pcicfg(dev)->reg32[reg / sizeof(uint32_t)]; +} + #endif /* !defined(__ROMCC__) */ #if CONFIG(MMCONF_SUPPORT) -- To view, visit https://review.coreboot.org/c/coreboot/+/36569 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iae54af09347d693620b631721576e4b916ea0f0f Gerrit-Change-Number: 36569 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 Nov 2019 16:33:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: Michael Niewöhner Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/apollolake: add soc implementation for ETR address API
by Aaron Durbin (Code Review) 04 Nov '19

04 Nov '19

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36566 ) Change subject: soc/intel/apollolake: add soc implementation for ETR address API ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/36566 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1832f5f14055fc3dbb502289035130ca7a5d6d33 Gerrit-Change-Number: 36566 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 04 Nov 2019 16:19:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: pmclib: add API to get ETR register address
by Aaron Durbin (Code Review) 04 Nov '19

04 Nov '19

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36565 ) Change subject: soc/intel/common: pmclib: add API to get ETR register address ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/36565/4/src/soc/intel/common/block… File src/soc/intel/common/block/include/intelblocks/pmclib.h: https://review.coreboot.org/c/coreboot/+/36565/4/src/soc/intel/common/block… PS4, Line 173: read_ Why is there a 'read' in this function? -- To view, visit https://review.coreboot.org/c/coreboot/+/36565 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I706f3e220d639a6133625e3cb7267f7009006af2 Gerrit-Change-Number: 36565 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 04 Nov 2019 16:19:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/hatch/variants/helios: Modify touchscreen power on sequence
by Paul Menzel (Code Review) 04 Nov '19

04 Nov '19

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35907 ) Change subject: mb/google/hatch/variants/helios: Modify touchscreen power on sequence ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/c/coreboot/+/35907/5//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/35907/5//COMMIT_MSG@13 PS5, Line 13: Add GPP_C4 setting. 1. Please mention the exact values you change it to. 2. Format this as a list? 3. Please mention where you got the correct values from? -- To view, visit https://review.coreboot.org/c/coreboot/+/35907 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I86c920ff1d5c0b510adde8a37f60003072d5f4e7 Gerrit-Change-Number: 35907 Gerrit-PatchSet: 5 Gerrit-Owner: Kane Chen <kane_chen(a)pegatron.corp-partner.google.com> Gerrit-Assignee: Shelley Chen <shchen(a)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: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> 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-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Mon, 04 Nov 2019 16:15:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2019