Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/26596 )
Change subject: util/docker: Attach docker-build-coreboot target to top level make
......................................................................
Abandoned
Meh
--
To view, visit https://review.coreboot.org/c/coreboot/+/26596
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I587b7d3cfce490c0b5b71928d90fbc1f4037d002
Gerrit-Change-Number: 26596
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alex Thiessen <alex.thiessen.de+coreboot(a)gmail.com>
Gerrit-MessageType: abandon
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29414 )
Change subject: src/soc/intel/braswell: Remove disabled LPE ACPI code
......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/29414/6//COMMIT_MSG
Commit Message:
https://review.coreboot.org/#/c/29414/6//COMMIT_MSG@12
PS6, Line 12: seperate
separate
--
To view, visit https://review.coreboot.org/c/coreboot/+/29414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic8acf9ea9e9b0ba9b272e20beb2023b7a4716a73
Gerrit-Change-Number: 29414
Gerrit-PatchSet: 6
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 09 Apr 2019 13:31:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29414 )
Change subject: src/soc/intel/braswell: Remove disabled LPE ACPI code
......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/29414/5/src/soc/intel/braswell/lpe.c
File src/soc/intel/braswell/lpe.c:
https://review.coreboot.org/#/c/29414/5/src/soc/intel/braswell/lpe.c@75
PS5, Line 75: file_name,
> I've never seen this kind of ssdt generation. […]
Using apcigen creates runtime decoding of static ACPI code.
From code it's not easy to read which ACPI code is generated.
For these reasons I choose this method.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic8acf9ea9e9b0ba9b272e20beb2023b7a4716a73
Gerrit-Change-Number: 29414
Gerrit-PatchSet: 6
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 09 Apr 2019 12:46:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: comment
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29414 )
Change subject: src/soc/intel/braswell: Remove disabled LPE ACPI code
......................................................................
Patch Set 6:
> Patch Set 6:
>
> One of these patches where already too many people
> stated their opinion (I'll try anyway). This often
> happens when the problem description is unclear.
>
> Frans, is this a cosmetic issue? that you just want
> to get rid of unneeded code? or do you experience
> any trouble because of the spurious code?
Experience slow system running Linux. Even reporting _STA disabled,
Linux remains requesting the _STA() of this device.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic8acf9ea9e9b0ba9b272e20beb2023b7a4716a73
Gerrit-Change-Number: 29414
Gerrit-PatchSet: 6
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 09 Apr 2019 12:37:41 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Akash Asthana has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27483 )
Change subject: sdm845: Add SPI QUP driver
......................................................................
Patch Set 46:
(7 comments)
Thanks for reviewing, I have shared the code with Mike, Once it's released here I will update in the buganizer.
https://review.coreboot.org/#/c/27483/46/src/mainboard/google/cheza/bootblo…
File src/mainboard/google/cheza/bootblock.c:
https://review.coreboot.org/#/c/27483/46/src/mainboard/google/cheza/bootblo…
PS46, Line 24: qup_spi_init(10, 1010 * KHz);
> Please add comments describing which devices (EC and H1) these are connected to.
Done
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/bootblock.c
File src/soc/qualcomm/sdm845/bootblock.c:
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/bootblock.…
PS46, Line 4: * Copyright (C) 2018-2019, The Linux Foundation. All rights reserved.
> nit: file not actually changing anymore?
We moved qup_spi_init from this file to cheza/bootblock.c in 2019.
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/include/so…
File src/soc/qualcomm/sdm845/include/soc/qcom_qup_se.h:
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/include/so…
PS46, Line 203: union proto_rx_trans_len {
> nit: is there a point in having this as a union? Why not just name the field tx_trans_len? (Goes for […]
I am not sure whether I get your suggestion correctly.
In union proto_rx_trans_len we have listed 3 registers 1) uart_tx_trans_len (No. of tx byte to transfer in UART) 2,3)i2c/spi_rx_trans_len (No. of rx byte to transfer in I2C/SPI).
>>Why not just name the field tx_trans_len?
I don't think that it would be appropriate, because we also use that same register to configure rx_trans_len for I2C/SPI. We need union for these cases where we use exactly same register in 2 very different context.
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c
File src/soc/qualcomm/sdm845/spi_qup.c:
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@…
PS46, Line 58: extern struct qup qup[16];
> This declaration should go in qcom_qup_se.h, not here.
Done
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@…
PS46, Line 195: setup_fifo_params(slave);
> Sorry if I asked this before and forgot again... […]
Yes, We need to do this again on every transfer. I have kept only the minimum neccassary configuration which we need to do for every transfer in this function.
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@…
PS46, Line 237: if (!(m_irq & M_CMD_DONE_EN) || ((m_irq & M_CMD_DONE_EN)
> !A || (A && B) can be simplified to !A || B. So this can just be […]
Done
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@…
PS46, Line 273: assert(((DEFAULT_SE_CLK - speed_hz * div) <= div * KHz) && (div > 0));
> nit: better put this at the top, right after div is calculated (before writing it to m_clk_cfg). […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/27483
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I35061727d5ccc550eaeb06caef4524bc4cf25b54
Gerrit-Change-Number: 27483
Gerrit-PatchSet: 46
Gerrit-Owner: T.Michael Turney <tturne(a)codeaurora.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Mukesh Savaliya <msavaliy(a)qualcomm.corp-partner.google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: T Michael Turney <mturney(a)codeaurora.org>
Gerrit-Reviewer: T.Michael Turney <tturne(a)codeaurora.org>
Gerrit-Reviewer: Vin Kamath <vink(a)codeaurora.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Akash Asthana <akashast(a)qualcomm.corp-partner.google.com>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Tue, 09 Apr 2019 06:49:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
nsekar(a)codeaurora.org has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/29963 )
Change subject: TEMP: NOT FOR REVIEW: qcs405: Adding the GPIO support for QCS405
......................................................................
Abandoned
Resolved by CB:29955
--
To view, visit https://review.coreboot.org/c/coreboot/+/29963
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ibdd675527458e597c1f49544425146bd17f28075
Gerrit-Change-Number: 29963
Gerrit-PatchSet: 15
Gerrit-Owner: nsekar(a)codeaurora.org
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: abandon
Akash Asthana has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29104 )
Change subject: sdm845: Port I2C driver
......................................................................
Patch Set 26:
(10 comments)
Thanks for reviewing, I have shared the code with Mike, Once it's released here I will update in the buganizer.
https://review.coreboot.org/#/c/29104/24/src/mainboard/google/cheza/mainboa…
File src/mainboard/google/cheza/mainboard.c:
https://review.coreboot.org/#/c/29104/24/src/mainboard/google/cheza/mainboa…
PS24, Line 42: i2c_init(12, 400 * KHz, 1);
> This should be an enum (QUP_WRAP1_S4 or something like that). […]
As per a discussion with clock team they need this enum(QUP_WRAP1_S4...) for their functionality. So, instead of moving this enum to qcom_qup_se.h, I have defined a new enum(qup_se) in the common header(qcom_qup_se.h) and using it across the drivers.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c
File src/soc/qualcomm/sdm845/i2c.c:
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@46
PS24, Line 46: unsigned int hz, unsigned int idx
> Let's not pass both Hz and an index. […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@101
PS24, Line 101: BITS_PER_WORD >> 4
> How exactly does this work? When it says byte_granularity I would expect 1 means 1 byte, 2 means 2 b […]
geni_byte_granularity is used to configure the number of protocol words in each FIFO entry.
Here is the mapping between granularity value and number of protocol words in each FIFO entry.
0 - 4*8, four words in each entry, max word size of 8 bits
1 - 2*16, two words in each entry, max word size of 16 bits
2 - 1*32, one word in each entry, max word size of 32 bits
3+ - undefined
It will work for BITS_PER_WORD == (16 or 32), however, in that case, we have to make necessary changes in FIFO PACKING CONFIGURATION because we have hardcoded the PACK_VECTORs assuming that BITS_PER_WORD is always 8.
It will not work for BITS_PER_WORD = 64.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@118
PS24, Line 118: setbits_le32(®s->geni_s_irq_enable, S_CMD_DONE_EN);
> Why not combine this with the other write to that register above?
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@122
PS24, Line 122: static u32 wait_till_irq_set(unsigned int bus)
> This is the exact same function as in the SPI code, right? Let's put it in qup. […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@187
PS24, Line 187: static void i2c_handle_error(unsigned int bus)
> Actually... […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@228
PS24, Line 228: stopwatch_init_msecs_expire(&sw, 1000);
> ... […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@245
PS24, Line 245: (m_irq & M_CMD_DONE_EN)
> nit: Can remove this part of the check. […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@247
PS24, Line 247: printk(BIOS_INFO, "%s:Error: Transfer failed\n", __func__);
> nit: Please write as "ERROR: %s failed\n". […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@261
PS24, Line 261: /* Set stretch = 0 for the last transfer */
> Please also explain what that bit does and why it needs to be set only for the last transfer.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/29104
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifb76564d8a11427423dd14d8ba7c8c7d500ef346
Gerrit-Change-Number: 29104
Gerrit-PatchSet: 26
Gerrit-Owner: T.Michael Turney <tturne(a)codeaurora.org>
Gerrit-Reviewer: Akash Asthana <akashast(a)qualcomm.corp-partner.google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: T Michael Turney <mturney(a)codeaurora.org>
Gerrit-Reviewer: T.Michael Turney <tturne(a)codeaurora.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 09 Apr 2019 06:48:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment