coreboot-gerrit May 2017

coreboot-gerrit@coreboot.org

1 participants
3888 discussions

Change in coreboot[master]: soc/intel/skylake: Use common systemagent code
by Subrata Banik (Code Review) 26 May '17

26 May '17

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/19796 to look at the new patch set (#3). Change subject: soc/intel/skylake: Use common systemagent code ...................................................................... soc/intel/skylake: Use common systemagent code This patch perform resource mapping for PCI, fixed MMIO, DRAM and IMR's based on inputs given by SoC. TEST=Ensure PCI root bridge 0:0:0 memory resource allocation remains same between previous implementation and current implementation. Change-Id: I93567a79b2d12dd5d6363957e55ce2cb86ff83a7 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/systemagent.c M src/soc/intel/skylake/systemagent.c 4 files changed, 54 insertions(+), 428 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/19796/3 -- To view, visit https://review.coreboot.org/19796 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I93567a79b2d12dd5d6363957e55ce2cb86ff83a7 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Subrata Banik <subrata.banik(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: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma(a)intel.com>

1 0

Change in coreboot[master]: soc/intel/common/block: Add Intel common systemagent support
by Subrata Banik (Code Review) 26 May '17

26 May '17

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/19668 ) Change subject: soc/intel/common/block: Add Intel common systemagent support ...................................................................... Patch Set 4: (42 comments) https://review.coreboot.org/#/c/19668/4//COMMIT_MSG Commit Message: PS4, Line 10: stemagent > systemagent ? sorry my bad https://review.coreboot.org/#/c/19668/4/src/soc/intel/apollolake/include/so… File src/soc/intel/apollolake/include/soc/systemagent.h: Line 47: #define SOC_SA_IMR_RESOURCES 20 > This seems odd. #define SOC_SA_IMR_RESOURCES 20 i have removed this as we are getting pointer to array so, size we are getting default. https://review.coreboot.org/#/c/19668/4/src/soc/intel/common/block/include/… File src/soc/intel/common/block/include/intelblocks/systemagent.h: PS4, Line 54: u32 > uintptr_t Done PS4, Line 55: u32 > uintptr_t Done PS4, Line 61: int > size_t Done PS4, Line 62: u32 > uintptr_t and pretty much every where else. Done Line 64: void enable_bios_reset_cpl(void); > A lot of these function names are too generic please if that. i thought those function names provide clear understanding about job description, any way, i'm bad at giving name, if you have some recommendation, please let me know. PS4, Line 66: #if ENV_RAMSTAGE > why are we guarding declarations? we can remove those guarding, but i thought of not including a code for a stage which not going to be in use for now from that stage. Move over this is the common header for all SA common code. Including the same for bootblock stage throwing error for device structure Line 87: const char *description; > You are going to need to add some documentation for this. i'm not sure, if we can add README kind of thing inside CB code. if so, i would rather prefer to mention for each IP to have the same README file. For an example: we would like to explain how this structure even work. i will add some comment here PS4, Line 92: > tabs instead of spaces? Nothing lines up Done https://review.coreboot.org/#/c/19668/4/src/soc/intel/common/block/systemag… File src/soc/intel/common/block/systemagent/systemagent.c: PS4, Line 27: #include <vendorcode/google/chromeos/chromeos.h> > remove Done PS4, Line 30: __attribute__((weak)); > This is only to make things link? If the SoCs are selecting this common blo yes, those are meant for linking between common code block and soc part. Now we don;t need this as we are calling soc to get array pointer PS4, Line 64: 1024 * 1024 > MiB and all the other places. Plus this base calculation is not very good. Done PS4, Line 81: u32 > uintptr_t Done PS4, Line 81: get_base_addr > These functions are named poorly for being global. They should be name spac i have added "sa_" before all SA functions similar to fast_spi PS4, Line 81: u32 > size_t Done PS4, Line 127: ARRAY_SIZE(sa_fixed_resources) > You should rely on the soc returning a pointer to the array and filling in Done PS4, Line 161: mask = ((1ULL<<20)-1); : mask = ~mask; > mask = ~(1ULL * MiB - 1); Done PS4, Line 166: !(entry->offset < 0) > >= 0 Done Line 167: base_addr = pci_read_config32(dev, entry->reg) & ~1; > Why isn't this using the get_base_addr() function? Done PS4, Line 178: (uint64_t) > no cast needed Done PS4, Line 180: (uint64_t) > no cast needed Done PS4, Line 207: #if IS_ENABLED(CONFIG_SA_ENABLE_DPR) > No #if guards. Done PS4, Line 241: #if IS_ENABLED(CONFIG_SA_ENABLE_DPR) > if (IS_ENABLED(...)) Done PS4, Line 245: uint32_t > uintptr_t Done Line 313: base_k = 4096 * 1024; /* 4GiB */ > 4 * (GiB / KiB) Done PS4, Line 314: >> 10 > / KiB Done PS4, Line 328: 0x100000 > (1*MiB - 0xc0000) / KiB Done PS4, Line 330: chromeos_reserve_ram_oops(dev, index++); > remove Done PS4, Line 335: #if IS_ENABLED(CONFIG_SA_ENABLE_IMR) > Don't guard code with #if Done PS4, Line 341: uint32_t > size_t Done PS4, Line 350: ((~mask & 0x0fffffff) + 1) > outside parens not needed Done PS4, Line 370: sa_imr_resources[i].index > Isn't this already 'offset' ? Done PS4, Line 375: << 10 > * KiB Done PS4, Line 376: << 10 > * KiB Done PS4, Line 399: : #if IS_ENABLED(CONFIG_SA_ENABLE_IMR) > if (IS_ENABLED(...)) Done https://review.coreboot.org/#/c/19668/4/src/soc/intel/common/block/systemag… File src/soc/intel/common/block/systemagent/systemagent_early.c: PS4, Line 28: !ENV_RAMSTAGE > Why does this need to be guarded? i had an opinion not to include any code which not getting used in that specific stage hence added guard. but more over, why i need this file even in ramstage because this file doing early SA init. its because of enable_bios_reset_cpl function getting called between different stage in SKL/KBL vs APL. hence i need to add complete file for ramstage. Also pci_io_write_config is bot available at ranstage, hence i need to limit with ENV_BOOTBLOCK token. if you think other wise, i can only guard bootblock_systemagent_early_init and open rest. PS4, Line 68: u32 > uintptr_t Done PS4, Line 68: set_base_addr > overly generic name can we make like sa_set_base_address() / hb_set_base_address() in that way it will sound more for SA or host bridge? PS4, Line 78: u32 > uintptr_t Done PS4, Line 97: int > size_t Done PS4, Line 117: int > size_t Done -- To view, visit https://review.coreboot.org/19668 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I969ff187e3d4199864cb2e9c9a13f4d04158e27c Gerrit-PatchSet: 4 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: V Sowmya <v.sowmya(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: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(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-HasComments: Yes

1 0

Change in coreboot[master]: soc/intel/skylake: Use common systemagent code
by Subrata Banik (Code Review) 26 May '17

26 May '17

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/19796 ) Change subject: soc/intel/skylake: Use common systemagent code ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/19796/2/src/soc/intel/skylake/romstage/syst… File src/soc/intel/skylake/romstage/systemagent.c: PS2, Line 28: \ > not needed. Done -- To view, visit https://review.coreboot.org/19796 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93567a79b2d12dd5d6363957e55ce2cb86ff83a7 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Subrata Banik <subrata.banik(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: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(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-HasComments: Yes

1 0

Change in coreboot[master]: soc/intel/apollolake: Use common systemagent code
by Subrata Banik (Code Review) 26 May '17

26 May '17

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/19795 ) Change subject: soc/intel/apollolake: Use common systemagent code ...................................................................... Patch Set 2: (6 comments) https://review.coreboot.org/#/c/19795/2/src/soc/intel/apollolake/northbridg… File src/soc/intel/apollolake/northbridge.c: Line 27: struct sa_mmio_descriptor sa_fixed_resources[SOC_SA_FIXED_RESOURCES] = { > const Done Line 32: struct sa_map_entry sa_memory_map[MAX_MAP_ENTRIES] = { > const Done Line 40: struct sa_mmio_descriptor sa_imr_resources[SOC_SA_IMR_RESOURCES] = { > const Done https://review.coreboot.org/#/c/19795/2/src/soc/intel/apollolake/romstage.c File src/soc/intel/apollolake/romstage.c: Line 84: struct sa_set_mmio_descriptor soc_fixed_set_resources[] = { > const Done PS2, Line 85: \ > '\' no needed. Done Line 86: "MCHBAR"}, > Space after " and before }. Also, Same line? Done -- To view, visit https://review.coreboot.org/19795 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15a3b2fc46ec9063b54379d41996b9a1d612cfd2 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Subrata Banik <subrata.banik(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: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(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-HasComments: Yes

1 0

Change in coreboot[master]: ec/librem/ec: Fix offset for Bluetooth enable (BTLE)
by Arthur Heymans (Code Review) 26 May '17

26 May '17

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/19897 ) Change subject: ec/librem/ec: Fix offset for Bluetooth enable (BTLE) ...................................................................... Patch Set 3: Code-Review+1 also ok on librem13v1? -- To view, visit https://review.coreboot.org/19897 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I68dc99e72a09f7affbcd691d03dd4607a898313e Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: mainboard/google/poppy: Enable H1 I2C TPM
by Furquan Shaikh (Code Review) 26 May '17

26 May '17

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19926 ) Change subject: mainboard/google/poppy: Enable H1 I2C TPM ...................................................................... Patch Set 2: Code-Review-1 -- To view, visit https://review.coreboot.org/19926 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c6c94fa05abf9f5374505ded5956e879ac79726 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: mainboard/google/poppy: Enable H1 I2C TPM
by build bot (Jenkins) (Code Review) 26 May '17

26 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19926 ) Change subject: mainboard/google/poppy: Enable H1 I2C TPM ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-checkpatch/10159/ : SUCCESS https://qa.coreboot.org/job/coreboot-gerrit/54437/ : SUCCESS -- To view, visit https://review.coreboot.org/19926 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c6c94fa05abf9f5374505ded5956e879ac79726 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: mainboard/google/poppy: Enable H1 I2C TPM
by Furquan Shaikh (Code Review) 26 May '17

26 May '17

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/19926 to look at the new patch set (#2). Change subject: mainboard/google/poppy: Enable H1 I2C TPM ...................................................................... mainboard/google/poppy: Enable H1 I2C TPM Enable H1 I2C TPM in Kconfig and devicetree for poppy. CQ-DEPEND=CL:513513,CL:*381534 BUG=b:36265511 BRANCH=None TEST=Compiles successfully. Change-Id: I4c6c94fa05abf9f5374505ded5956e879ac79726 Signed-off-by: Furquan Shaikh <furquan(a)chromium.org> --- M src/mainboard/google/poppy/Kconfig M src/mainboard/google/poppy/variants/baseboard/devicetree.cb 2 files changed, 8 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/19926/2 -- To view, visit https://review.coreboot.org/19926 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4c6c94fa05abf9f5374505ded5956e879ac79726 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in coreboot[master]: mainboard/google/poppy: Enable H1 I2C TPM
by build bot (Jenkins) (Code Review) 26 May '17

26 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19926 ) Change subject: mainboard/google/poppy: Enable H1 I2C TPM ...................................................................... Patch Set 1: Verified-1 Build Unstable https://qa.coreboot.org/job/coreboot-gerrit/54436/ : UNSTABLE https://qa.coreboot.org/job/coreboot-checkpatch/10158/ : SUCCESS -- To view, visit https://review.coreboot.org/19926 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c6c94fa05abf9f5374505ded5956e879ac79726 Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: mainboard/google/poppy: Enable H1 I2C TPM
by Furquan Shaikh (Code Review) 26 May '17

26 May '17

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19926 ) Change subject: mainboard/google/poppy: Enable H1 I2C TPM ...................................................................... Patch Set 1: Code-Review-1 Not yet ready to push this in. Have to verify this first. -- To view, visit https://review.coreboot.org/19926 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c6c94fa05abf9f5374505ded5956e879ac79726 Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-HasComments: No

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2017