Attention is currently required from: Angel Pons, Nico Huber.
Hello Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/82763?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review+2 by Angel Pons, Verified+1 by build bot (Jenkins)
Change subject: sb/intel/smbus: Implement smbus_send_byte()
......................................................................
sb/intel/smbus: Implement smbus_send_byte()
Allows to use this driver for the SMBus console without sending an index
byte for every sent char (i.e. !CONSOLE_I2C_SMBUS_HAVE_DATA_REGISTER).
Tested with WiP VIA CX700-M2 port and FT4222H as receiver.
Change-Id: Ic368ef379039b104064c9a91474b188646388dd2
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
---
M src/southbridge/intel/common/smbus.c
1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/82763/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/82763?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ic368ef379039b104064c9a91474b188646388dd2
Gerrit-Change-Number: 82763
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Attention is currently required from: Alper Nebi Yasak.
Martin L Roth has posted comments on this change by Alper Nebi Yasak. ( https://review.coreboot.org/c/coreboot/+/82059?usp=email )
Change subject: mainboard/qemu-aarch64: Set CONFIG_PCI_IOBASE to 0x3eff0000
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Leaving this open because you mentioned we could read the info from dtb instead. […]
Let's merge this for now, and if it gets updated to read the value later, that's great.
--
To view, visit https://review.coreboot.org/c/coreboot/+/82059?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I85439ba68740d64f789983b37d9c95f849ce4f72
Gerrit-Change-Number: 82059
Gerrit-PatchSet: 2
Gerrit-Owner: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Comment-Date: Tue, 09 Jul 2024 15:23:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Attention is currently required from: Arthur Heymans, Maximilian Brune, Philipp Hug, Ron Minnich.
Felix Held has posted comments on this change by Maximilian Brune. ( https://review.coreboot.org/c/coreboot/+/83098?usp=email )
Change subject: arch/riscv: Factor out common romstage code
......................................................................
Patch Set 5:
(1 comment)
File src/mainboard/sifive/hifive-unmatched/romstage.c:
https://review.coreboot.org/c/coreboot/+/83098/comment/d288444c_37f572d6?us… :
PS5, Line 19: console_init();
> UART subsystem depends on `clock_init()`. […]
doing that in a separate patch sounds good to me, since it's a separate logical change. i'd prefer to have that patch before this one though
--
To view, visit https://review.coreboot.org/c/coreboot/+/83098?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ieb11d2644bf42dacf89ef15b2ec51286fe729d64
Gerrit-Change-Number: 83098
Gerrit-PatchSet: 5
Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx>
Gerrit-Reviewer: Ron Minnich <rminnich(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Philipp Hug <philipp(a)hug.cx>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Ron Minnich <rminnich(a)gmail.com>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:51:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82762?usp=email )
Change subject: console/i2c_smbus: Allow to send data w/o register offset
......................................................................
Patch Set 1:
(1 comment)
File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/82762/comment/268fa246_8b31b81b?us… :
PS1, Line 322: bool "Write to a specific data register"
> I wouldn't know why. It's generic console code, at least I think it is.
I suggested this because the option below mentions a logging device. In any case, I am resolving this because I don't care about it.
--
To view, visit https://review.coreboot.org/c/coreboot/+/82762?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1c714768fdd4aea4885e40a85d21fa42414ce32c
Gerrit-Change-Number: 82762
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:38:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Attention is currently required from: Angel Pons.
Nico Huber has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82762?usp=email )
Change subject: console/i2c_smbus: Allow to send data w/o register offset
......................................................................
Patch Set 1:
(1 comment)
File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/82762/comment/36ed3f59_305226b6?us… :
PS1, Line 322: bool "Write to a specific data register"
> nit: […]
I wouldn't know why. It's generic console code, at least I think it is.
--
To view, visit https://review.coreboot.org/c/coreboot/+/82762?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1c714768fdd4aea4885e40a85d21fa42414ce32c
Gerrit-Change-Number: 82762
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:28:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82772?usp=email )
Change subject: cpu/via/c7: Compress ramstage with LZ4 by default
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/82772/comment/c89a0399_5cb7c68e?us… :
PS2, Line 9: It's a slow CPU.
How much faster is LZ4 vs LZMA?
--
To view, visit https://review.coreboot.org/c/coreboot/+/82772?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I0bf75f410c1d9134f05a2d11b8d011499a7cf794
Gerrit-Change-Number: 82772
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:27:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82771?usp=email )
Change subject: cpu/via/c7: Use the simple p4-netburst CAR teardown
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/82771?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Icba7586145fbfd859d738ecd7a407739a7024ebb
Gerrit-Change-Number: 82771
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:26:14 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82770?usp=email )
Change subject: nb/via/cx700: Implement raminit
......................................................................
Patch Set 2: Code-Review+2
(7 comments)
File src/northbridge/via/cx700/chip.h:
https://review.coreboot.org/c/coreboot/+/82770/comment/75b5bf43_9166ea06?us… :
PS2, Line 15: mem_clocks
At first, I read this as "mem**e**_clocks"...
File src/northbridge/via/cx700/raminit.h:
https://review.coreboot.org/c/coreboot/+/82770/comment/73b04f18_8838ff72?us… :
PS2, Line 7: void sdram_enable(const struct dram_cfg *);
Should be easy to fix.
File src/northbridge/via/cx700/raminit.c:
https://review.coreboot.org/c/coreboot/+/82770/comment/c2a3dc95_fe3a2980?us… :
PS2, Line 79: #define GET_SPD(i, val, tmp, reg) \
Why is this not a function?
https://review.coreboot.org/c/coreboot/+/82770/comment/afe4620a_dd711f7f?us… :
PS2, Line 213: ((NA_ODT << 6) | (NA_ODT << 4) | (NA_ODT << 2) | Rank0_ODT)
A helper macro to factor out the shifts would probably make this table a bit easier to read. No big deal though.
https://review.coreboot.org/c/coreboot/+/82770/comment/677b1fb1_3676bf15?us… :
PS2, Line 276:
Is this a tab?
```suggestion
/* Chipset Performance UP and other setting after DRAM Sizing Registers */
```
https://review.coreboot.org/c/coreboot/+/82770/comment/d7ed441c_c03119c0?us… :
PS2, Line 361: /* FIXME: Only supports 2 ranks per DIMM */
Can there be more?
https://review.coreboot.org/c/coreboot/+/82770/comment/0b958aa0_34c76a64?us… :
PS2, Line 1274:
: write32p(0, 0x55555555);
: write32p(4, 0x55555555);
: udelay(15);
: if (read32p(0) != 0x55555555)
: break;
: if (read32p(4) != 0x55555555)
: break;
: write32p(0, 0xaaaaaaaa);
: write32p(4, 0xaaaaaaaa);
: udelay(15);
: if (read32p(0) != 0xaaaaaaaa)
: break;
: if (read32p(4) != 0xaaaaaaaa)
: break;
Could be factored out into a helper function:
```
bool test_ram_rw(u32 val)
{
write32p(0, val);
write32p(4, val);
udelay(15);
return read32p(0) == val && read32p(4) == val;
}
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/82770?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ibc36b4f314cdf47f18c8be0fcb98218c50938e94
Gerrit-Change-Number: 82770
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:26:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82769?usp=email )
Change subject: nb/via/cx700: Implement FSB tuning
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/82769/comment/2353a83b_d912b188?us… :
PS2, Line 17: seems unclear if it was put there intentionally.
If this soft reset doesn't clear the FSB settings, I imagine it's to help commit the settings. I've seen similar stuff for Intel QPI.
File src/northbridge/via/cx700/romstage.c:
https://review.coreboot.org/c/coreboot/+/82769/comment/723c1aae_fbd6b196?us… :
PS2, Line 47: pci_write_config8(_sdev_host_ctrl, 0x4f, 0x01);
Wasn't there one of these in bootblock? Or does it need to be done again?
--
To view, visit https://review.coreboot.org/c/coreboot/+/82769?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I24ba6cfaab2ca3069952a6c399a065caea7b49f2
Gerrit-Change-Number: 82769
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:15:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82768?usp=email )
Change subject: nb/via/cx700: Perform early bootblock init
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/82768/comment/fd63fda8_24ce0a2e?us… :
PS2, Line 9: Disable a timer (GP3) that is always running by default. And enable
What does GP3 do if not disabled?
--
To view, visit https://review.coreboot.org/c/coreboot/+/82768?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I77f179433b280d67860fc495605b5764ed081a6c
Gerrit-Change-Number: 82768
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Tue, 09 Jul 2024 14:12:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes