[coreboot-gerrit] Change in ...coreboot[master]: sb/intel/i82801gx: Autodisable functions based on devicetree

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30244 )

Change subject: sb/intel/i82801gx: Autodisable functions based on devicetree
......................................................................


Patch Set 8:

(8 comments)

https://review.coreboot.org/#/c/30244/8//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30244/8//COMMIT_MSG@12 
PS8, Line 12: This mostly reimplements what sandybridge does albeit a bit simpler.
As usual, I would love to see common code. Any reason not to share code
with sandy?


https://review.coreboot.org/#/c/30244/8//COMMIT_MSG@14 
PS8, Line 14: TESTED with PCIe coalescing (root port 0 disabled in devicetree)
This is confusing because you reuse the term "coalescing" but what you
do is only fixing a missing function 0, not filling every potential gap.
Which is fine but should probably have a better name.


https://review.coreboot.org/#/c/30244/8/src/northbridge/intel/i945/early_init.c 
File src/northbridge/intel/i945/early_init.c:

https://review.coreboot.org/#/c/30244/8/src/northbridge/intel/i945/early_init.c@181 
PS8, Line 181: 	RCBA32_OR(FD, 1);
hmmm, do it in southbridge_intel_i82801gx_ops.init()?


https://review.coreboot.org/#/c/30244/8/src/southbridge/intel/i82801gx/i82801gx.h 
File src/southbridge/intel/i82801gx/i82801gx.h:

https://review.coreboot.org/#/c/30244/8/src/southbridge/intel/i82801gx/i82801gx.h@241 
PS8, Line 241: #define RPFN_FNMASK(port)       (7 << ((port) * 4))
tabs please


https://review.coreboot.org/#/c/30244/8/src/southbridge/intel/i82801gx/i82801gx.h@285 
PS8, Line 285: #define ICH_DISABLE_PCIE(x)	(1 << (16 + x))
parentheses around (x)

why not name it FD_PCIE(x)?

The numbers are off-by-one compared to the explicitly named versions... are the latter still needed at all?


https://review.coreboot.org/#/c/30244/8/src/southbridge/intel/i82801gx/i82801gx.h@296 
PS8, Line 296: #define ICH_DISABLE_UHCI(x)	(1 << (8 + x))
same here


https://review.coreboot.org/#/c/30244/8/src/southbridge/intel/i82801gx/i82801gx.c 
File src/southbridge/intel/i82801gx/i82801gx.c:

https://review.coreboot.org/#/c/30244/8/src/southbridge/intel/i82801gx/i82801gx.c@77 
PS8, Line 77: 	for (dev = all_devices; dev; dev = dev->next) {
it should be enough to walk bus 0?


https://review.coreboot.org/#/c/30244/8/src/southbridge/intel/i82801gx/i82801gx.c@155 
PS8, Line 155: 		pci_write_config32(dev, PCI_COMMAND, reg32);
Hmmm, move the whole thing into ich_hide_devfn()?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/30244
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I83576599538a02d295fe00b35826f98d8c97d1cf
Gerrit-Change-Number: 30244
Gerrit-PatchSet: 8
Gerrit-Owner: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Alexander Couzens <lynxis at fe80.eu>
Gerrit-Reviewer: Angel Pons <th3fanbus at gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Damien Zammit <damien at zamaudio.com>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas at noos.fr>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro at das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Peter Lemenkov <lemenkov at gmail.com>
Gerrit-Comment-Date: Tue, 18 Dec 2018 21:44:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181218/f4bfe79d/attachment.html>