coreboot-gerrit May 2017

coreboot-gerrit@coreboot.org

1 participants
3888 discussions

Change in coreboot[master]: console/flashsconsole: Add spi flash console for debugging
by Youness Alaoui (Code Review) 24 May '17

24 May '17

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/19849 ) Change subject: console/flashsconsole: Add spi flash console for debugging ...................................................................... Patch Set 2: (13 comments) tx_flush support added here : https://review.coreboot.org/19865 I'm working now on adding fmap support. https://review.coreboot.org/#/c/19849/2//COMMIT_MSG Commit Message: PS2, Line 7: Adding > Add Done PS2, Line 23: skylake > Intel Skylake Done PS2, Line 26: fmap > FMAP Done https://review.coreboot.org/#/c/19849/2/src/console/Kconfig File src/console/Kconfig: PS2, Line 242: region > It's called "file" in CBFS (and I think "section" is more common for FMAP). Ah yes, 'file' for CBFS, The API cbfs_locate_file_in_region() and the 'region_device' struct confused me in thinking it's called a 'region'. For FMAP, I used the 'area' naming because of the fmap_locate_area() API. Line 258: region in the CBFS. > Why define two mechanisms? The FMAP seems like a more appropriate place to Two mechanisms because I don't really know much about how FMAP works, so I originally did it for CBFS (it was called cbfsconsole and renamed the APIs just yesterday). Aaron Durbin suggested FMAP is more appropriate, so I made it fmap-compatible (but not yet implemented), but there's no reason to throw about the cbfs-related code. Also, I think newbies (like me) will find it easier to do a 'cbfstool extract' than having to define a custom fmap, set it in the config, then use dd with specific offsets to copy out the console file from the rom. So.. using CBFS is just more convenient, and that's what I would use (and possibly other newbies), but Aaron (and possibly more advanced coreboot devs) would prefer to use FMAP. Line 260: if CONSOLE_SPI_FLASH > Just use depends on instead of this block. Great idea. I copied this from the CONSOLE_CBMEM which uses an 'if'. https://review.coreboot.org/#/c/19849/2/src/drivers/spi/Makefile.inc File src/drivers/spi/Makefile.inc: Line 22: | tr '\000' '\377' > $@ > Can't this go on a single line? Or at least the full dd piece? Yes, the bs= was the long $(shell printf) line, which is why it was originally split. Once I moved it out into a variable, it became easier to fit in a single line. https://review.coreboot.org/#/c/19849/2/src/drivers/spi/flashconsole.c File src/drivers/spi/flashconsole.c: Line 41: if (cbfs_boot_locate(&file, "console", NULL) == 0) { > nit: probably looks better with Will do, but I personally prefer it the way I did it. And no lines go over 80 chars, so it's your gerrit I suppose. Line 55: boot_device_init(); > Ya. I think we have almost every call that would touch the boot device in t Yes, I know, but there's no guarantee that the prepare_program_locate API is calling it. Also, there's no guarantee the prepare_program_locate will be called/remain there (especially if we add FMAP support), so I preferred to keep the boot_device_init() call right next to the boot_device_rw() call in case of refactor. It's a no-op and it's safer, so let's keep it (also noticed it being redundantly called in existing parts of the code). Line 60: * the end of a previous log write. > You're banking on the fact that console output can never have legitimate 0x That's a good point, and yes, I am assuming the console is for plaintext only, and any binary is printed using the hexdump APIs. Is that not the case? If not, then I can do it the way you suggested. Line 121: for (i = 0; i < LINE_BUFFER_SIZE - 1; i++) { > Why not just store the current index in a global as well, rather than searc No idea. will do. thx Line 129: if (len >= LINE_BUFFER_SIZE - 1 || > Why not tie this to tx_flush instead? I copied the spiconsole.c as a base and it didn't have tx_flush, so I didn't think of it. I think adding tx_flush support in a separate commit would be better. https://review.coreboot.org/#/c/19849/2/src/include/console/flash.h File src/include/console/flash.h: Line 25: #define __CONSOLE_FLASH_ENABLE__ IS_ENABLED(CONFIG_CONSOLE_SPI_FLASH) > All the consoles that are actually used then... Yeah, all the other consoles I saw used it this way, it's usually also used with (&& ENV_RAMSTAGE), stuff like that. And I used it a lot when the flashconsole was not working in romstage, then the expression got simplified to this. -- To view, visit https://review.coreboot.org/19849 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I74a297b94f6881d8c27cbe5168f161d8331c3df3 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: Yes

1 0

Change in coreboot[master]: ec/lenovo/h8/h8: Always enable tp-smapi and thermal
by build bot (Jenkins) (Code Review) 24 May '17

24 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19864 ) Change subject: ec/lenovo/h8/h8: Always enable tp-smapi and thermal ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-checkpatch/9974/ : SUCCESS https://qa.coreboot.org/job/coreboot-gerrit/54244/ : SUCCESS -- To view, visit https://review.coreboot.org/19864 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5aa50e0b1148a7cc8e51480623368ee62edb849 Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: console/flashconsole: Add support for tx_flush
by Youness Alaoui (Code Review) 24 May '17

24 May '17

Youness Alaoui has uploaded a new change for review. ( https://review.coreboot.org/19865 ) Change subject: console/flashconsole: Add support for tx_flush ...................................................................... console/flashconsole: Add support for tx_flush Support flushing the flash console so non \n terminated lines can be printed to flash. Also use it internally when the line buffering is done. Change-Id: I6516ecefddf676bdac6fb97536a08a5c5ecc0d4d Signed-off-by: Youness Alaoui <youness.alaoui(a)puri.sm> --- M src/console/console.c M src/drivers/spi/flashconsole.c M src/include/console/flash.h 3 files changed, 46 insertions(+), 15 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/19865/1 diff --git a/src/console/console.c b/src/console/console.c index 2b7f99d..877c8dc 100644 --- a/src/console/console.c +++ b/src/console/console.c @@ -63,6 +63,7 @@ __uart_tx_flush(); __ne2k_tx_flush(); __usb_tx_flush(); + __flashconsole_tx_flush(); } void console_write_line(uint8_t *buffer, size_t number_of_bytes) diff --git a/src/drivers/spi/flashconsole.c b/src/drivers/spi/flashconsole.c index 29680a2..b9b8877 100644 --- a/src/drivers/spi/flashconsole.c +++ b/src/drivers/spi/flashconsole.c @@ -98,7 +98,30 @@ memset(line_buffer, 0, LINE_BUFFER_SIZE); // Set g_rdev last so tx_byte doesn't get executed early car_set_var(g_rdev, rdev); +} +static const struct region_device *_tx_flush(const struct region_device *rdev) +{ + uint8_t *line_buffer = car_get_var_ptr(g_line_buffer); + uint32_t offset = car_get_var(g_offset); + uint32_t region_offset = car_get_var(g_region_offset); + uint32_t region_size = car_get_var(g_region_size); + uint32_t len = car_get_var(g_line_offset); + + // We crop the rest of the line if the region is full + if (offset + len >= region_size) + len = region_size - offset; + + rdev_writeat(rdev, line_buffer, region_offset + offset, len); + + // If the region is full, stop future write attempts + if (offset + len >= region_size) + rdev = NULL; + + car_set_var(g_offset, offset + len); + car_set_var(g_line_offset, 0); + + return rdev; } void flashconsole_tx_byte(unsigned char c) @@ -123,22 +146,23 @@ if (len >= LINE_BUFFER_SIZE - 1 || offset + len >= region_size || c == '\n') { - uint32_t region_offset = car_get_var(g_region_offset); - - // We crop the rest of the line if the region is full - if (offset + len >= region_size) - len = region_size - offset; - - rdev_writeat(rdev, line_buffer, region_offset + offset, - len); - - // If the region is full, stop future write attempts - if (offset + len >= region_size) - rdev = NULL; - - car_set_var(g_offset, offset + len); - car_set_var(g_line_offset, 0); + rdev = _tx_flush(rdev); } car_set_var(g_rdev, rdev); } + +} + +void flashconsole_tx_flush(void) +{ + const struct region_device *rdev = car_get_var(g_rdev); + + if (rdev) { + /* Prevent any recursive loops in case the spi flash driver + * calls printk (in case of transaction timeout or + * any other error while writing) */ + car_set_var(g_rdev, NULL); + rdev = _tx_flush(rdev); + car_set_var(g_rdev, rdev); + } } diff --git a/src/include/console/flash.h b/src/include/console/flash.h index fa4de00..7bf0c93 100644 --- a/src/include/console/flash.h +++ b/src/include/console/flash.h @@ -21,6 +21,7 @@ void flashconsole_init(void); void flashconsole_tx_byte(unsigned char c); +void flashconsole_tx_flush(void); #define __CONSOLE_FLASH_ENABLE__ IS_ENABLED(CONFIG_CONSOLE_SPI_FLASH) @@ -30,9 +31,14 @@ { flashconsole_tx_byte(data); } +static inline void __flashconsole_tx_flush(void) +{ + flashconsole_tx_flush(); +} #else static inline void __flashconsole_init(void) {} static inline void __flashconsole_tx_byte(u8 data) {} +static inline void __flashconsole_tx_flush(void) {} #endif /* __CONSOLE_FLASH_ENABLE__ */ -- To view, visit https://review.coreboot.org/19865 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6516ecefddf676bdac6fb97536a08a5c5ecc0d4d Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Youness Alaoui <snifikino(a)gmail.com>

1 0

Change in coreboot[master]: console/flashsconsole: Add spi flash console for debugging
by Youness Alaoui (Code Review) 24 May '17

24 May '17

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/19849 to look at the new patch set (#3). Change subject: console/flashsconsole: Add spi flash console for debugging ...................................................................... console/flashsconsole: Add spi flash console for debugging If CONSOLE_SPI_FLASH config is enabled, we can write the cbmem messages to the 'console' region in CBFS which allows us to grab the log when we read the flash. This is useful when you don't have usb debugging, and UART lines are hard to find. Since a failure to boot would require a hardware flasher anyways, we can get the log at the same time. This feature should only be used when no alternative is found and only when we can't boot the system, because excessive writes to the flash is not recommended. This has been tested on purism/librem13 v2 and librem 15 v3 which run Intel Skylake hardware. It has not been tested on other archs or with a driver other than the fast_spi. The Kconfig help mentions support for writing to an FMAP area but this isn't supported yet. Change-Id: I74a297b94f6881d8c27cbe5168f161d8331c3df3 Signed-off-by: Youness Alaoui <youness.alaoui(a)puri.sm> --- M src/console/Kconfig M src/console/console.c M src/drivers/spi/Makefile.inc A src/drivers/spi/flashconsole.c A src/include/console/flash.h 5 files changed, 245 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/19849/3 -- To view, visit https://review.coreboot.org/19849 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I74a297b94f6881d8c27cbe5168f161d8331c3df3 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in coreboot[master]: ec/lenovo/h8/h8: Always enable tp-smapi and thermal
by Patrick Rudolph (Code Review) 24 May '17

24 May '17

Patrick Rudolph has uploaded a new change for review. ( https://review.coreboot.org/19864 ) Change subject: ec/lenovo/h8/h8: Always enable tp-smapi and thermal ...................................................................... ec/lenovo/h8/h8: Always enable tp-smapi and thermal Always enable tp-smapi and thermal managment. It should always be enabled as the SMM code relies on it. Change-Id: Ia5aa50e0b1148a7cc8e51480623368ee62edb849 Signed-off-by: Patrick Rudolph <siro(a)das-labor.org> --- M src/ec/lenovo/h8/h8.c 1 file changed, 9 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/19864/1 diff --git a/src/ec/lenovo/h8/h8.c b/src/ec/lenovo/h8/h8.c index b259d9a..5ea4a2c 100644 --- a/src/ec/lenovo/h8/h8.c +++ b/src/ec/lenovo/h8/h8.c @@ -209,21 +209,25 @@ { struct ec_lenovo_h8_config *conf = dev->chip_info; u8 val; - u8 beepmask0, beepmask1, config1; + u8 beepmask0, beepmask1, reg8; dev->ops = &h8_dev_ops; h8_log_ec_version(); - ec_write(H8_CONFIG0, conf->config0); - config1 = conf->config1; + /* Always enable I/O range 0x1600-0x160f and thermal management */ + reg8 = conf->config0; + reg8 |= H8_CONFIG0_SMM_H8_ENABLE; + reg8 |= H8_CONFIG0_TC_ENABLE; + ec_write(H8_CONFIG0, reg8); + reg8 = conf->config1; if (conf->has_keyboard_backlight) { if (get_option(&val, "backlight") != CB_SUCCESS) val = 0; /* Both backlights. */ - config1 = (config1 & 0xf3) | ((val & 0x3) << 2); + reg8 = (reg8 & 0xf3) | ((val & 0x3) << 2); } - ec_write(H8_CONFIG1, config1); + ec_write(H8_CONFIG1, reg8); ec_write(H8_CONFIG2, conf->config2); ec_write(H8_CONFIG3, conf->config3); -- To view, visit https://review.coreboot.org/19864 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia5aa50e0b1148a7cc8e51480623368ee62edb849 Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org>

1 0

Change in coreboot[master]: util/abuild: Start junit testcase block on kconfig failure
by Patrick Georgi (Code Review) 24 May '17

24 May '17

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/19861 ) Change subject: util/abuild: Start junit testcase block on kconfig failure ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/19861 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f9083c346ac7b6502f854b7e1f1054e81954d76 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: 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-HasComments: No

1 0

Change in coreboot[master]: soc/amd/common: Add tseg size to Kconfig
by Martin Roth (Code Review) 24 May '17

24 May '17

Martin Roth has posted comments on this change. ( https://review.coreboot.org/19750 ) Change subject: soc/amd/common: Add tseg size to Kconfig ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/19750/2//COMMIT_MSG Commit Message: PS2, Line 7: common stonyridge -- To view, visit https://review.coreboot.org/19750 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I26f3b06f85f0cf5613656c1d5df55bd9ea4bbbbc Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.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]: soc/amd/stoneyridge: Add CPU files
by Martin Roth (Code Review) 24 May '17

24 May '17

Martin Roth has posted comments on this change. ( https://review.coreboot.org/19723 ) Change subject: soc/amd/stoneyridge: Add CPU files ...................................................................... Patch Set 3: (6 comments) https://review.coreboot.org/#/c/19723/3/src/northbridge/amd/pi/Kconfig File src/northbridge/amd/pi/Kconfig: Line 19: default y if SOC_AMD_PI > Why are enabling northbridge/amd/pi under these circumstances? What's the l Agreed - If we're moving things to SOC, why would we still need pieces in src/northbridge? Isn't the whole point of the SOC that it's all self-contained? https://review.coreboot.org/#/c/19723/3/src/soc/amd/common/Kconfig File src/soc/amd/common/Kconfig: Line 13 As mentioned in the previous commit: You might want to avoid the issues that are being seen currently in soc/intel/common and not have this Kconfig file here. The issue is that chips in soc/amd/[ab].* will be able to override defaults set in this file, but Kconfig files that get sourced later (soc/amd/[d-z].*) will NOT be able to override these defaults. Because we're automatically sourcing soc/*/*/Kconfig, the order here can't be changed easily except by not having a Kconfig file in src/amd/common. My recommendation would be to move this Kconfig (or this section of the Kconfig) to soc/amd/common/pi and source src/amd/common/*/Kconfig from the top level Kconfig at an appropriate point. https://review.coreboot.org/#/c/19723/3/src/soc/amd/common/Makefile.inc File src/soc/amd/common/Makefile.inc: Line 13 I'd also recommend we avoid having files in soc/amd/common, and move them all into subdirectories for the the same reason. If we have files here, people are going to want a Kconfig here, and as previously mentioned, that creates problems. https://review.coreboot.org/#/c/19723/3/src/soc/amd/common/cache_as_ram.inc File src/soc/amd/common/cache_as_ram.inc: PS3, Line 149: change these tabs to spaces? https://review.coreboot.org/#/c/19723/3/src/vendorcode/amd/Kconfig File src/vendorcode/amd/Kconfig: PS3, Line 4: 7 Updating Sage's copyright? https://review.coreboot.org/#/c/19723/3/src/vendorcode/amd/pi/Kconfig File src/vendorcode/amd/pi/Kconfig: Line 29: if CPU_AMD_PI_00630F01 || CPU_AMD_PI_00730F01 || CPU_AMD_PI_00670F00_FP4 || CPU_AMD_PI_00670F00_FT4 || CPU_AMD_PI_00660F01 || SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 Wrap the line? if CPU_AMD_PI_00630F01 || CPU_AMD_PI_00730F01 || \ CPU_AMD_PI_00670F00_FP4 || CPU_AMD_PI_00670F00_FT4 || \ CPU_AMD_PI_00660F01 || SOC_AMD_STONEYRIDGE_FP4 || \ SOC_AMD_STONEYRIDGE_FT4 -- To view, visit https://review.coreboot.org/19723 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8b6b1991372c2c6a02709777a73615a86e78ac26 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: Yes

1 0

Change in coreboot[master]: mainboard/google/eve: Update VR config settings
by build bot (Jenkins) (Code Review) 24 May '17

24 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19826 ) Change subject: mainboard/google/eve: Update VR config settings ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-checkpatch/9973/ : SUCCESS https://qa.coreboot.org/job/coreboot-gerrit/54243/ : SUCCESS -- To view, visit https://review.coreboot.org/19826 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I274245821f68fb3151e5563ea0c75eaa1ad32c08 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc: Add AMD StoneyRidge southbridge code
by Martin Roth (Code Review) 24 May '17

24 May '17

Martin Roth has posted comments on this change. ( https://review.coreboot.org/19722 ) Change subject: soc: Add AMD StoneyRidge southbridge code ...................................................................... Patch Set 3: (8 comments) https://review.coreboot.org/#/c/19722/3/src/soc/amd/common/Kconfig File src/soc/amd/common/Kconfig: Line 5 You might want to avoid the issues that are being seen currently in soc/intel/common and not have this Kconfig file here. The issue is that chips in soc/amd/[ab].* will be able to override defaults set in this file, but Kconfig files that get sourced later (soc/amd/[d-z].*) will NOT be able to override these defaults. Because we're automatically sourcing soc/*/*/Kconfig, the order here can't be changed easily except by not having a Kconfig file in src/amd/common. I'd recommend not having any files in this directory, but if we do, you could help things by adding a comment here: # Caution: Do NOT add any default values to this file. # Default values here cannot be overridden by Kconfig files # that are sourced later in the soc/amd directory. My recommendation would be to move this Kconfig file to soc/amd/common/util or soc/amd/common/devices and source src/amd/common/*/Kconfig from the top level Kconfig at an appropriate point. https://review.coreboot.org/#/c/19722/3/src/soc/amd/common/amd_pci_util.h File src/soc/amd/common/amd_pci_util.h: PS3, Line 16: AMD_PCI_UTIL_H Update to SOC_AMD_PCI_UTIL_H? https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/chip.c File src/soc/amd/stoneyridge/chip.c: Line 60: extra line https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/soc… File src/soc/amd/stoneyridge/include/soc/hudson.h: PS3, Line 17: STONEYRIDGE_H Should the name of the file be updated? https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/soc… File src/soc/amd/stoneyridge/include/soc/pci_devs.h: PS3, Line 74: #if IS_ENABLED(CONFIG_SOUTHBRIDGE_AMD_PI_BOLTON) : #define IDE_DEV 0x14 : #define IDE_FUNC 1 : #define IDE_DEVID 0x780C : #define IDE_DEVFN PCI_DEVFN(IDE_DEV,IDE_FUNC) : #endif remove? PS3, Line 106: #if IS_ENABLED(CONFIG_SOUTHBRIDGE_AMD_PI_BOLTON) : #define SB_PCIE_DEV 0x15 : #define SB_PCIE_PORT1_FUNC 0 : #define SB_PCIE_PORT2_FUNC 1 : #define SB_PCIE_PORT3_FUNC 2 : #define SB_PCIE_PORT4_FUNC 3 : #define SB_PCIE_PORT1_DEVID 0x7820 : #define SB_PCIE_PORT2_DEVID 0x7821 : #define SB_PCIE_PORT3_DEVID 0x7822 : #define SB_PCIE_PORT4_DEVID 0x7823 : #define SB_PCIE_PORT1_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT1_FUNC) : #define SB_PCIE_PORT2_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT2_FUNC) : #define SB_PCIE_PORT3_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT3_FUNC) : #define SB_PCIE_PORT4_DEVFN PCI_DEVFN(SB_PCIE_DEV,SB_PCIE_PORT4_FUNC) : #endif remove? https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/soc… File src/soc/amd/stoneyridge/include/soc/smi.h: PS3, Line 8: _SOUTHBRIDGE_AMD_PI_STONEYRIDGE_SMI_H _SOC_AMD_PI_STONEYRIDGE_SMI_H And could we be consistent about the naming on these? Some have no leading or trailing underscores, some have one, some have both. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/smbus.c File src/soc/amd/stoneyridge/smbus.c: PS3, Line 16: #ifndef _STONEYRIDGE_SMBUS_C_ : #define _STONEYRIDGE_SMBUS_C_ Let's not add another place where we're including a c file. These shouldn't be needed. -- To view, visit https://review.coreboot.org/19722 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib88a868e654ad127be70ecc506f6b90b784f8d1b Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: Yes

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2017