coreboot-gerrit May 2017

coreboot-gerrit@coreboot.org

1 participants
3888 discussions

Change in coreboot[master]: soc/intel/common: Add sanity check of PCR_BASE_ADDRESS
by build bot (Jenkins) (Code Review) 13 May '17

13 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19684 ) Change subject: soc/intel/common: Add sanity check of PCR_BASE_ADDRESS ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-checkpatch/9390/ : SUCCESS https://qa.coreboot.org/job/coreboot-gerrit/53606/ : SUCCESS -- To view, visit https://review.coreboot.org/19684 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I396aa1a3e89507c90e17229a986de5d2c0887c9c Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Lijian Zhao <lijian.zhao(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc/intel/common: Add sanity check of PCR_BASE_ADDRESS
by Lijian Zhao (Code Review) 13 May '17

13 May '17

Lijian Zhao has uploaded a new change for review. ( https://review.coreboot.org/19684 ) Change subject: soc/intel/common: Add sanity check of PCR_BASE_ADDRESS ...................................................................... soc/intel/common: Add sanity check of PCR_BASE_ADDRESS PCR_BASE_ADRESS may be zero if SOC Kconfig didn't define the non zero default value. TEST=Remove the PCR_BASE_ADDRESS config in Apollolake Kconfig file and build. BUG=None Change-Id: I396aa1a3e89507c90e17229a986de5d2c0887c9c Signed-off-by: Lijian Zhao <lijian.zhao(a)intel.com> --- M src/soc/intel/common/block/pcr/pcr.c 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/19684/1 diff --git a/src/soc/intel/common/block/pcr/pcr.c b/src/soc/intel/common/block/pcr/pcr.c index 6560a4b..ccd7d4d 100644 --- a/src/soc/intel/common/block/pcr/pcr.c +++ b/src/soc/intel/common/block/pcr/pcr.c @@ -24,6 +24,9 @@ /* Create an address based off of port id and offset. */ reg_addr = CONFIG_PCR_BASE_ADDRESS; + #if(CONFIG_PCR_BASE_ADDRESS ==0) + #error "PCR_BASE_ADDRESS need to be non-zero!" + #endif reg_addr += ((uintptr_t)pid) << PCR_PORTID_SHIFT; reg_addr += (uintptr_t)offset; -- To view, visit https://review.coreboot.org/19684 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I396aa1a3e89507c90e17229a986de5d2c0887c9c Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Lijian Zhao <lijian.zhao(a)intel.com>

1 0

Change in coreboot[master]: rockchip/rk3399: Add MIPI driver
by Julius Werner (Code Review) 13 May '17

13 May '17

Julius Werner has posted comments on this change. ( https://review.coreboot.org/19477 ) Change subject: rockchip/rk3399: Add MIPI driver ...................................................................... Patch Set 5: (2 comments) https://review.coreboot.org/#/c/19477/5/src/soc/rockchip/rk3399/mipi.c File src/soc/rockchip/rk3399/mipi.c: Line 429: mdelay(12); ...and, actually, when I'm looking at the kernel patch mentioned below more closely it does: 1. regulator enabling [already done for us by the time we reach this function] 2. several msleep(12) [presumably waiting for the regulators, we might be able to skip that here] 3. EXIT_SLEEP_MODE 4. msleep(120) 5. SET_DISPLAY_ON [and that's it, I'm not sure where it does the VID_MODE... probably later] So, are you sure that you really got the delays in the right place here? Should the msleep(120) be between EXIT_SLEEP_MODE and SET_DISPLAY_ON instead, maybe? Looks to me like either the kernel code or this patch is wrong. Line 432: mdelay(120); > Done You said "Done" but you didn't really answer my question... that's really the more important part. We need to know what this number represents to be able to implement it in the right way. In https://patchwork.kernel.org/patch/9642191/ it seems that this is called "T6"... is that a number from the panel datasheet? (I don't have one unfortunately...) If this *is* panel dependent, then the number must be in devicetree.cb like all the other panel values. You'd need to add a new field for this to chip.h, then read that in display.c and pass it into this function as an additional parameter. (If, on the other hand, it is not panel dependent and would be the same for every panel on this SoC, it's okay to hardcode it.) -- To view, visit https://review.coreboot.org/19477 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I02475eefb187c619c614b1cd20e97074bc8d917f Gerrit-PatchSet: 5 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: nickey.yang(a)rock-chips.com Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Sean Paul <seanpaul(a)chromium.org> Gerrit-Reviewer: Shunqian Zheng <zhengsq(a)rock-chips.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: nickey.yang(a)rock-chips.com Gerrit-HasComments: Yes

1 0

Change in coreboot[master]: util/superiotool/fintek.c: Add support for F71889A
by Felix Held (Code Review) 13 May '17

13 May '17

Felix Held has posted comments on this change. ( https://review.coreboot.org/17442 ) Change subject: util/superiotool/fintek.c: Add support for F71889A ...................................................................... Patch Set 9: (6 comments) https://review.coreboot.org/#/c/17442/9/util/superiotool/fintek.c File util/superiotool/fintek.c: PS9, Line 475: 0x00 one bit of the register 0x28 is determined by some hardware strap, probably NANA would be better here PS9, Line 499: NANA the value for 0xa6 should probably be 0x00 PS9, Line 491: {0x06, "GPIO", : {0xf0, 0xf1, 0xf2, 0xf3, 0xfe, 0xff, 0xe0, 0xe1, 0xe2, : 0xe3, 0xd0, 0xd1, 0xd2, 0xd3, 0xc0, 0xc1, 0xc2, 0xc3, : 0xb0, 0xb1, 0xb2, 0xa0, 0xa1, 0xa2, 0xa4, 0xa5, 0xa6, : 0xab, 0xac, 0xad, 0xae, 0xaf, 0x90, 0x91, 0x92, 0x93, : 0x80, 0x81, 0x82, 0x83, EOT}, : {0x00, 0x7f, NANA, 0x00, 0x00, 0x00, 0x00, 0x7f, NANA, : 0x00, 0x00, 0xe0, NANA, 0x00, 0x00, 0xff, NANA, 0x00, : 0x00, 0xff, NANA, 0x00, 0x1f, NANA, 0x00, 0x00, NANA, : 0x00, 0x00, 0x00, 0xe0, 0x00, 0x00, 0x00, NANA, 0x00, : 0x00, 0xff, NANA, 0x00, EOT} }, ordering these so that the addresses increase and are not in a slightly random order would probably be a good idea PS9, Line 505: MISC I'm unsure about this byte; might also be 0x00, but it isn't really documented PS9, Line 517: 0x07 0x07 or 0x00 that's the question. the datasheet has both values for this register... PS9, Line 512: {0x0a, "PME, ACPI, and ERP", : {0x30, 0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, : 0xfa, 0xfc, 0xfe, 0xe0, 0xe1, 0xe2, 0xe3, 0xe4, 0xe5, : 0xe6, 0xe7, 0xe8, 0xe9, 0xec, 0xed, 0xee, EOT}, : {0x00, 0x00, NANA, 0x00, NANA, 0x26, 0x04, 0x07, 0x08, : 0x00, 0x07, 0x00, 0x80, 0xcc, NANA, 0x13, 0x09, 0xc7, : 0x09, 0x63, 0x10, 0xff, NANA, 0x00, 0x00, EOT} }, maybe order the address/value pairs. address 0xfd with data 0x00 missing -- To view, visit https://review.coreboot.org/17442 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5035ebe2c1c66213069567da757886923e5596 Gerrit-PatchSet: 9 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marty Plummer <ntzrmtthihu777(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marty Plummer <ntzrmtthihu777(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: Yes

1 0

Change in coreboot[master]: util/cbmem: mmap underflow on low addresses
by Furquan Shaikh (Code Review) 13 May '17

13 May '17

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19679 ) Change subject: util/cbmem: mmap underflow on low addresses ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/19679 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I32b0c15b2f1aa43fc57656d5d2d5f0e4e90e94ef Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: superio/fintek: Add support for Fintek F71889A.
by Felix Held (Code Review) 13 May '17

13 May '17

Felix Held has posted comments on this change. ( https://review.coreboot.org/17669 ) Change subject: superio/fintek: Add support for Fintek F71889A. ...................................................................... Patch Set 9: > or for that matter, why 0x07ff signifies one byte the leftmost '1' bit says what the highest address is that can be assigned to a peripheral in the io space. the number of zeros from the right side says how much bytes are needed to be allocated for the peripheral; the zeros on the right are basically the address mask, that masks the 2^n bytes being reserved with N being the number of zeros. Maybe have a look at the function pnp_get_ioresource in pnp_device.c and/or diagram in f71869ad/superio.c -- To view, visit https://review.coreboot.org/17669 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I91c60a3b48cd4872ae7a27de8f49faa40e877a27 Gerrit-PatchSet: 9 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marty Plummer <ntzrmtthihu777(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Reviewer: Marty Plummer <ntzrmtthihu777(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: superio/fintek: Add support for Fintek F71889A.
by Felix Held (Code Review) 13 May '17

13 May '17

Felix Held has posted comments on this change. ( https://review.coreboot.org/17669 ) Change subject: superio/fintek: Add support for Fintek F71889A. ...................................................................... Patch Set 9: Code-Review+1 (1 comment) https://review.coreboot.org/#/c/17669/9/src/superio/fintek/f71889a/superio.c File src/superio/fintek/f71889a/superio.c: PS9, Line 57: 0x07f8 After thinking a bit about the problem here, I'd say that 0x7ff is still the better option here. Both options have their downsides. The keyboard controller uses two addresses in the io space, 0x60 and 0x64. I have no idea what the reasons were behind this decision, but that's what we have to deal with. If a system also has an embedded controller (EC), the EC uses the io addresses 0x62 and 0x66. Again, don't ask me who thought that this is a good idea. In this SIO chip there is only one register for setting the base io address for the keyboard controller and the second address is automatically the address of the first one plus 4 bytes. When using 0x7ff here, only the io space for the first register will be reserved. If someone maps another peripheral to the address 0x64, you might see some weird effects. So the developers have to make sure to use sane values here, but that's something I consider acceptable. When using 0x7f8, the io space for bot keyboard controller registers is reserved, but (and that's the problem in systems that also have an EC) the range also covers the addresses of the EC, so that will probably fail. I didn't think about this when I recommended using 0x7f8 as a mask here. So all in all I'd use 0x7ff here, since the side effects of this are less bad. tl;dr: I'd use 0x7ff here -- To view, visit https://review.coreboot.org/17669 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I91c60a3b48cd4872ae7a27de8f49faa40e877a27 Gerrit-PatchSet: 9 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marty Plummer <ntzrmtthihu777(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Reviewer: Marty Plummer <ntzrmtthihu777(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: Yes

1 0

Change in coreboot[master]: AGESA binaryPI: Consolidate ACPI for IMC
by Philipp Deppenwiese (Code Review) 13 May '17

13 May '17

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/18538 ) Change subject: AGESA binaryPI: Consolidate ACPI for IMC ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/18538 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieff6041f3c9ad02f9cebae0ec83d0898abb0d601 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: 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: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc/intel/common/block:[WIP] Add GPIO common code
by build bot (Jenkins) (Code Review) 13 May '17

13 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/18917 ) Change subject: soc/intel/common/block:[WIP] Add GPIO common code ...................................................................... Patch Set 22: Build Successful https://qa.coreboot.org/job/coreboot-gerrit/53605/ : SUCCESS -- To view, visit https://review.coreboot.org/18917 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3a1d56df46668bfb08206ca4a99202db5cd1da7c Gerrit-PatchSet: 22 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Brenton Dong <brenton.m.dong(a)intel.com> Gerrit-Reviewer: Dhaval Sharma <dhaval.v.sharma(a)intel.com> Gerrit-Reviewer: Divya Chellappa <divya.chellappa(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Han Lim Ng <nhlhanlim93(a)gmail.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Lijian Zhao <lijian.zhao(a)intel.com> Gerrit-Reviewer: Marc Herbert <marc.herbert(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: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: sowmya v <sowmyav235(a)gmail.com> Gerrit-HasComments: No

1 0

Change in coreboot[master]: [RFC]mb/*/*: Remove rtc nvram configurable baud rate
by Martin Roth (Code Review) 13 May '17

13 May '17

Martin Roth has posted comments on this change. ( https://review.coreboot.org/19682 ) Change subject: [RFC]mb/*/*: Remove rtc nvram configurable baud rate ...................................................................... Patch Set 4: (1 comment) I'm good with removing this. It seems like we'd generally want to use the fastest possible speed for the serial port anyway. https://review.coreboot.org/#/c/19682/4/src/mainboard/via/epia-m700/driving… File src/mainboard/via/epia-m700/driving_clk_phase_data.c: Line 22 Unrelated? -- To view, visit https://review.coreboot.org/19682 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I71698d9b188eeac73670b18b757dff5fcea0df41 Gerrit-PatchSet: 4 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: Yes

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2017