Attention is currently required from: Nicholas Chin, ZhiYuanNJ.
Anastasia Klimchuk has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82776?usp=email )
Change subject: ch347_spi: Add spi clock frequency selection
......................................................................
Patch Set 11:
(2 comments)
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/82776/comment/07382b7a_6f740b48?us… :
PS11, Line 291: speed_index = 1;
Default speed_index also needs to be 2 (to index the entry 15M)
I just thought, you now have two values, `divisor` and `speed_index` which are meant to be in sync, but it's easy to get them out of sync and this can be potential bug.
Maybe you can leave `speed_index` only? with default 2.
> int speed_index = 2; /* default 15M SPI */
And then below, just always use `spispeeds[speed_index].divisor`
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/82776/comment/6f90e76f_4c64de52?us… :
PS11, Line 1025: The default SPI speed is 30MHz if no value is specified.
Since we are returning back to 15M, you need to update here as well
> The default SPI speed is 15MHz if no value is specified (which corresponds to ``spispeed=15M``).
--
To view, visit https://review.coreboot.org/c/flashrom/+/82776?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: If2be48929db540a6598ac0b60b37e64597156db7
Gerrit-Change-Number: 82776
Gerrit-PatchSet: 11
Gerrit-Owner: ZhiYuanNJ
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: ZhiYuanNJ
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 10:01:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Anastasia Klimchuk, Hsuan Ting Chen, Hsuan-ting Chen, Kane Chen, Peter Marheine, Subrata Banik.
Hello Anastasia Klimchuk, Hsuan Ting Chen, Hsuan-ting Chen, Peter Marheine, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/83896?usp=email
to look at the new patch set (#7).
The following approvals got outdated and were removed:
Code-Review+1 by Hsuan-ting Chen, Code-Review+1 by Subrata Banik, Code-Review+2 by Peter Marheine, Verified+1 by build bot (Jenkins)
Change subject: chipset_enable.c: Use PCI_ACCESS_ECAM to access pci register
......................................................................
chipset_enable.c: Use PCI_ACCESS_ECAM to access pci register
In the latest pciutils(v3.13.0), it supports accessing pci registers
by ecam. This patch uses libpci version check to decide whether
flashrom calls libpci and use 0xcf8/0xcfc or ecam to access pci
registers.
BUG=b:359813524
TEST=with libpci >= 3.13.0, flashrom is working with ECAM access
Change-Id: I4549f87c8b01da0a1d3d8ce0b3b75c1f5fa2cbab
Signed-off-by: Kane Chen <kane.chen(a)intel.com>
---
M chipset_enable.c
M doc/release_notes/devel.rst
M meson.build
3 files changed, 22 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/83896/7
--
To view, visit https://review.coreboot.org/c/flashrom/+/83896?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I4549f87c8b01da0a1d3d8ce0b3b75c1f5fa2cbab
Gerrit-Change-Number: 83896
Gerrit-PatchSet: 7
Gerrit-Owner: Kane Chen <kane.chen(a)intel.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Attention: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Kane Chen <kane.chen(a)intel.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Hsuan Ting Chen, Kane Chen.
Hsuan-ting Chen has posted comments on this change by Kane Chen. ( https://review.coreboot.org/c/flashrom/+/83896?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: chipset_enable.c: Use PCI_ACCESS_ECAM to access pci register
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/83896/comment/7921fe4e_de626aec?us… :
PS6, Line 943: msg_gdbg("Using libpci PCI_ACCESS_ECAM\n");
: #else
: pci_acc->method = PCI_ACCESS_I386_TYPE1;
: msg_gdbg("Using libpci PCI_ACCESS_I386_TYPE1\n");
nit: This file typically uses msg_pdbg(), could we use that for consistency?
--
To view, visit https://review.coreboot.org/c/flashrom/+/83896?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I4549f87c8b01da0a1d3d8ce0b3b75c1f5fa2cbab
Gerrit-Change-Number: 83896
Gerrit-PatchSet: 6
Gerrit-Owner: Kane Chen <kane.chen(a)intel.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Attention: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Attention: Kane Chen <kane.chen(a)intel.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 19 Aug 2024 05:31:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Anastasia Klimchuk, Nicholas Chin.
ZhiYuanNJ has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82776?usp=email )
Change subject: ch347_spi: Add spi clock frequency selection
......................................................................
Patch Set 11:
(1 comment)
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/82776/comment/5824beba_d69ff458?us… :
PS7, Line 365: 30MHz
> I have a suggestion, but please tell me what you all think about it. […]
I agree with this idea because the default value cannot be determined at the moment. I suggest keeping the original default value. I have already made the modifications.
--
To view, visit https://review.coreboot.org/c/flashrom/+/82776?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: If2be48929db540a6598ac0b60b37e64597156db7
Gerrit-Change-Number: 82776
Gerrit-PatchSet: 11
Gerrit-Owner: ZhiYuanNJ
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 02:07:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: ZhiYuanNJ
Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com>
Attention is currently required from: Hsuan Ting Chen, Hsuan-ting Chen, Kane Chen, Kane Chen, Subrata Banik.
Hello Hsuan Ting Chen, Hsuan-ting Chen, Peter Marheine, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/83896?usp=email
to look at the new patch set (#6).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: chipset_enable.c: Use PCI_ACCESS_ECAM to access pci register
......................................................................
chipset_enable.c: Use PCI_ACCESS_ECAM to access pci register
In the latest pciutils(v3.13.0), it supports accessing pci registers
by ecam. This patch uses libpci version check to decide whether
flashrom calls libpci and use 0xcf8/0xcfc or ecam to access pci
registers.
BUG=b:359813524
TEST=with libpci >= 3.13.0, flashrom is working with ECAM access
Change-Id: I4549f87c8b01da0a1d3d8ce0b3b75c1f5fa2cbab
Signed-off-by: Kane Chen <kane.chen(a)intel.com>
---
M chipset_enable.c
M doc/release_notes/devel.rst
M meson.build
3 files changed, 22 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/83896/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/83896?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I4549f87c8b01da0a1d3d8ce0b3b75c1f5fa2cbab
Gerrit-Change-Number: 83896
Gerrit-PatchSet: 6
Gerrit-Owner: Kane Chen <kane.chen(a)intel.com>
Gerrit-Reviewer: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Attention: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Kane Chen <kane.chen(a)intel.corp-partner.google.com>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Kane Chen <kane.chen(a)intel.com>