coreboot-gerrit May 2017

coreboot-gerrit@coreboot.org

1 participants
3888 discussions

Change in coreboot[master]: include/device: Add pci ids for Intel SD
by build bot (Jenkins) (Code Review) 31 May '17

31 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19997 ) Change subject: include/device: Add pci ids for Intel SD ...................................................................... Patch Set 4: Build Successful https://qa.coreboot.org/job/coreboot-gerrit/54653/ : SUCCESS https://qa.coreboot.org/job/coreboot-checkpatch/10362/ : SUCCESS -- To view, visit https://review.coreboot.org/19997 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iee966991ffd0f73cee9520c85eeba6530e224457 Gerrit-PatchSet: 4 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc/intel/apollolake: Remove soc/pci_ids dependency
by build bot (Jenkins) (Code Review) 31 May '17

31 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19999 ) Change subject: soc/intel/apollolake: Remove soc/pci_ids dependency ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-gerrit/54654/ : SUCCESS -- To view, visit https://review.coreboot.org/19999 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8101fe52591b09caadfe104ca8daab4258837c7 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc/intel/apollolake: Remove soc/pci_ids dependency
by build bot (Jenkins) (Code Review) 31 May '17

31 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19999 ) Change subject: soc/intel/apollolake: Remove soc/pci_ids dependency ...................................................................... Patch Set 1: Verified-1 Build Failed https://qa.coreboot.org/job/coreboot-gerrit/54652/ : ABORTED https://qa.coreboot.org/job/coreboot-checkpatch/10361/ : SUCCESS -- To view, visit https://review.coreboot.org/19999 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8101fe52591b09caadfe104ca8daab4258837c7 Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc/intel/apollolake: Remove soc/pci_ids dependency
by Hannah Williams (Code Review) 31 May '17

31 May '17

Hannah Williams has uploaded a new patch set (#2). ( https://review.coreboot.org/19999 ) Change subject: soc/intel/apollolake: Remove soc/pci_ids dependency ...................................................................... soc/intel/apollolake: Remove soc/pci_ids dependency and add pci ids for GLK and APL from device/pci_ids.h Change-Id: If8101fe52591b09caadfe104ca8daab4258837c7 Signed-off-by: Hannah Williams <hannah.williams(a)intel.com> --- M src/soc/intel/apollolake/dsp.c M src/soc/intel/apollolake/graphics.c M src/soc/intel/apollolake/lpc.c M src/soc/intel/apollolake/northbridge.c M src/soc/intel/apollolake/p2sb.c M src/soc/intel/apollolake/pmc.c M src/soc/intel/apollolake/sd.c M src/soc/intel/apollolake/sram.c 8 files changed, 52 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/19999/2 -- To view, visit https://review.coreboot.org/19999 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If8101fe52591b09caadfe104ca8daab4258837c7 Gerrit-PatchSet: 2 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>

1 0

Change in coreboot[master]: soc/intel/apollolake: Remove soc/pci_ids dependency and add ...
by Hannah Williams (Code Review) 31 May '17

31 May '17

Hannah Williams has uploaded a new change for review. ( https://review.coreboot.org/19999 ) Change subject: soc/intel/apollolake: Remove soc/pci_ids dependency and add pci ids for GLK and APL from device/pci_ids.h ...................................................................... soc/intel/apollolake: Remove soc/pci_ids dependency and add pci ids for GLK and APL from device/pci_ids.h Change-Id: If8101fe52591b09caadfe104ca8daab4258837c7 Signed-off-by: Hannah Williams <hannah.williams(a)intel.com> --- M src/soc/intel/apollolake/dsp.c M src/soc/intel/apollolake/graphics.c M src/soc/intel/apollolake/lpc.c M src/soc/intel/apollolake/northbridge.c M src/soc/intel/apollolake/p2sb.c M src/soc/intel/apollolake/pmc.c M src/soc/intel/apollolake/sd.c M src/soc/intel/apollolake/sram.c 8 files changed, 52 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/19999/1 diff --git a/src/soc/intel/apollolake/dsp.c b/src/soc/intel/apollolake/dsp.c index a2d21aa..76547ee 100644 --- a/src/soc/intel/apollolake/dsp.c +++ b/src/soc/intel/apollolake/dsp.c @@ -16,7 +16,6 @@ #include <device/device.h> #include <device/pci.h> #include <device/pci_ids.h> -#include <soc/pci_ids.h> static struct device_operations dsp_dev_ops = { .read_resources = &pci_dev_read_resources, @@ -25,8 +24,14 @@ .scan_bus = &scan_static_bus, }; +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_APL_AUDIO, + PCI_DEVICE_ID_INTEL_GLK_AUDIO, + 0, +}; + static const struct pci_driver apollolake_dsp __pci_driver = { .ops = &dsp_dev_ops, .vendor = PCI_VENDOR_ID_INTEL, - .device = PCI_DEVICE_ID_APOLLOLAKE_AUDIO + .devices = pci_device_ids, }; diff --git a/src/soc/intel/apollolake/graphics.c b/src/soc/intel/apollolake/graphics.c index 9963d2c..7123e4f 100644 --- a/src/soc/intel/apollolake/graphics.c +++ b/src/soc/intel/apollolake/graphics.c @@ -23,7 +23,6 @@ #include <device/pci.h> #include <device/pci_ids.h> #include <soc/pci_devs.h> -#include <soc/pci_ids.h> #include <soc/intel/common/opregion.h> uintptr_t fsp_soc_get_igd_bar(void) @@ -91,8 +90,9 @@ }; static const unsigned short pci_device_ids[] = { - PCI_DEVICE_ID_APOLLOLAKE_IGD_HD_505, - PCI_DEVICE_ID_APOLLOLAKE_IGD_HD_500, + PCI_DEVICE_ID_INTEL_APL_IGD_HD_505, + PCI_DEVICE_ID_INTEL_APL_IGD_HD_500, + PCI_DEVICE_ID_INTEL_GLK_IGD, 0, }; diff --git a/src/soc/intel/apollolake/lpc.c b/src/soc/intel/apollolake/lpc.c index 810c4c9..67c87b1 100644 --- a/src/soc/intel/apollolake/lpc.c +++ b/src/soc/intel/apollolake/lpc.c @@ -23,7 +23,6 @@ #include <pc80/mc146818rtc.h> #include <soc/acpi.h> #include <soc/lpc.h> -#include <soc/pci_ids.h> #include <soc/pm.h> #include <vboot/vbnv.h> @@ -167,8 +166,14 @@ .scan_bus = scan_lpc_bus, }; +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_APL_LPC, + PCI_DEVICE_ID_INTEL_GLK_LPC, + 0, +}; + static const struct pci_driver soc_lpc __pci_driver = { .ops = &device_ops, .vendor = PCI_VENDOR_ID_INTEL, - .device = PCI_DEVICE_ID_APOLLOLAKE_LPC, + .devices = pci_device_ids, }; diff --git a/src/soc/intel/apollolake/northbridge.c b/src/soc/intel/apollolake/northbridge.c index 9519603..4997c6f 100644 --- a/src/soc/intel/apollolake/northbridge.c +++ b/src/soc/intel/apollolake/northbridge.c @@ -21,7 +21,6 @@ #include <device/pci.h> #include <device/pci_ids.h> #include <soc/systemagent.h> -#include <soc/pci_ids.h> static uint32_t get_bar(device_t dev, unsigned int index) { @@ -162,8 +161,14 @@ .enable = DEVICE_NOOP }; +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_APL_NB, + PCI_DEVICE_ID_INTEL_GLK_NB, + 0, +}; + static const struct pci_driver northbridge_driver __pci_driver = { .ops = &northbridge_ops, .vendor = PCI_VENDOR_ID_INTEL, - .device = PCI_DEVICE_ID_APOLLOLAKE_NB + .devices = pci_device_ids, }; diff --git a/src/soc/intel/apollolake/p2sb.c b/src/soc/intel/apollolake/p2sb.c index 8056d14..f0e584b 100644 --- a/src/soc/intel/apollolake/p2sb.c +++ b/src/soc/intel/apollolake/p2sb.c @@ -21,7 +21,6 @@ #include <rules.h> #include <soc/iomap.h> #include <soc/pci_devs.h> -#include <soc/pci_ids.h> #include <soc/p2sb.h> #define P2SB_E0 0xe0 @@ -67,8 +66,14 @@ .set_resources = DEVICE_NOOP, }; +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_APL_P2SB, + PCI_DEVICE_ID_INTEL_GLK_P2SB, + 0, +}; + static const struct pci_driver pmc __pci_driver = { .ops = &device_ops, .vendor = PCI_VENDOR_ID_INTEL, - .device = PCI_DEVICE_ID_APOLLOLAKE_P2SB, + .devices = pci_device_ids, }; diff --git a/src/soc/intel/apollolake/pmc.c b/src/soc/intel/apollolake/pmc.c index ef39630..150f7ce 100644 --- a/src/soc/intel/apollolake/pmc.c +++ b/src/soc/intel/apollolake/pmc.c @@ -21,7 +21,6 @@ #include <console/console.h> #include <cpu/x86/smm.h> #include <soc/iomap.h> -#include <soc/pci_ids.h> #include <soc/gpio.h> #include <soc/pci_devs.h> #include <soc/pm.h> @@ -164,8 +163,14 @@ .init = &pmc_init, }; +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_APL_PMC, + PCI_DEVICE_ID_INTEL_GLK_PMC, + 0, +}; + static const struct pci_driver pmc __pci_driver = { .ops = &device_ops, .vendor = PCI_VENDOR_ID_INTEL, - .device = PCI_DEVICE_ID_APOLLOLAKE_PMC, + .devices= pci_device_ids, }; diff --git a/src/soc/intel/apollolake/sd.c b/src/soc/intel/apollolake/sd.c index 2f38061..e2b5ab4 100644 --- a/src/soc/intel/apollolake/sd.c +++ b/src/soc/intel/apollolake/sd.c @@ -75,8 +75,14 @@ #endif }; +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_APL_SD, + PCI_DEVICE_ID_INTEL_GLK_SD, + 0, +}; + static const struct pci_driver pch_sd __pci_driver = { .ops = &dev_ops, .vendor = PCI_VENDOR_ID_INTEL, - .device = 0x5aca + .devices= pci_device_ids, }; diff --git a/src/soc/intel/apollolake/sram.c b/src/soc/intel/apollolake/sram.c index 44eb0ee..0225327 100644 --- a/src/soc/intel/apollolake/sram.c +++ b/src/soc/intel/apollolake/sram.c @@ -17,7 +17,6 @@ #include <device/device.h> #include <device/pci.h> #include <device/pci_ids.h> -#include <soc/pci_ids.h> #include <soc/pci_devs.h> #include <soc/iomap.h> @@ -61,8 +60,14 @@ .enable_resources = pci_dev_enable_resources, }; +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_APL_SRAM, + PCI_DEVICE_ID_INTEL_GLK_SRAM, + 0, +}; + static const struct pci_driver pmc __pci_driver = { .ops = &device_ops, .vendor = PCI_VENDOR_ID_INTEL, - .device = PCI_DEVICE_ID_APOLLOLAKE_SRAM, + .devices= pci_device_ids, }; -- To view, visit https://review.coreboot.org/19999 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If8101fe52591b09caadfe104ca8daab4258837c7 Gerrit-PatchSet: 1 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Hannah Williams <hannah.williams(a)intel.com>

1 0

Change in coreboot[master]: include/device: Add pci ids for Intel SD
by Hannah Williams (Code Review) 31 May '17

31 May '17

Hello Aaron Durbin, Bora Guvendik, build bot (Jenkins), Furquan Shaikh, I'd like you to reexamine a change. Please visit https://review.coreboot.org/19997 to look at the new patch set (#4). Change subject: include/device: Add pci ids for Intel SD ...................................................................... include/device: Add pci ids for Intel SD Change-Id: Iee966991ffd0f73cee9520c85eeba6530e224457 Signed-off-by: Bora Guvendik <bora.guvendik(a)intel.com> --- M src/include/device/pci_ids.h 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/19997/4 -- To view, visit https://review.coreboot.org/19997 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iee966991ffd0f73cee9520c85eeba6530e224457 Gerrit-PatchSet: 4 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in coreboot[master]: soc/intel/common/block: Add Intel common systemagent support
by Furquan Shaikh (Code Review) 31 May '17

31 May '17

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19668 ) Change subject: soc/intel/common/block: Add Intel common systemagent support ...................................................................... Patch Set 5: (12 comments) https://review.coreboot.org/#/c/19668/5/src/soc/intel/common/block/include/… File src/soc/intel/common/block/include/intelblocks/systemagent.h: PS5, Line 77: int What are the expected return values? I believe you are only checking for 0/non-zero? PS5, Line 74: struct sa_mmio_descriptor { : unsigned int index; : size_t size; : int (*get_resource)(device_t dev, unsigned int index, : uintptr_t *base, size_t *size); : const char *description; : }; The other mmio_descriptor above is named _set_. Should this be _get_. Is there a way the two can be combined into just one? PS5, Line 83: int Why not unsigned? PS5, Line 85: int bool? PS5, Line 86: int bool? PS5, Line 124: int uintptr_t? Can you please add comments as to what these functions are expected to return? https://review.coreboot.org/#/c/19668/5/src/soc/intel/common/block/systemag… File src/soc/intel/common/block/systemagent/systemagent.c: PS5, Line 84: if(*len) { How would this ever evaluate to false? You are assigning *len in the switch block above even in default case. PS5, Line 93: len > i guess this is a miss, we don't need to get the len, purpose for this API It's confusing. The above function is also named as _get_*_base_addr but sets len, but this function does not. PS5, Line 197: value |= ~mask; value &= ~mask; PS5, Line 251: sa_read_map_entries(dev, sa_map_entry, &mc_values[0]); : sa_report_map_entries(dev, sa_map_entry, &mc_values[0]); Why not report it while you are actually reading it? PS5, Line 397: int index; : : index = sa_add_fixed_mmio_resources(dev, sa_fixed_resources, count); : : return index; return sa_add_fixed_mmio_resources(dev, sa_fixed_resources, count); PS5, Line 407: int resource_index; : : resource_index = sa_add_dram_resources(dev, index, sa_map_entry); : : return resource_index; return sa_add_dram_resources(dev, index, sa_map_entry); -- To view, visit https://review.coreboot.org/19668 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I969ff187e3d4199864cb2e9c9a13f4d04158e27c Gerrit-PatchSet: 5 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: Barnali Sarkar <barnali.sarkar(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma(a)intel.com> Gerrit-HasComments: Yes

1 0

Change in coreboot[master]: include/device: Add pci ids for Intel SD
by Furquan Shaikh (Code Review) 31 May '17

31 May '17

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19997 ) Change subject: include/device: Add pci ids for Intel SD ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/19997 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iee966991ffd0f73cee9520c85eeba6530e224457 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: cbfstool/lz4/lib/lz4frame.c: Don't fallthrough on case dstag...
by build bot (Jenkins) (Code Review) 31 May '17

31 May '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19998 ) Change subject: cbfstool/lz4/lib/lz4frame.c: Don't fallthrough on case dstage_getHeader ...................................................................... Patch Set 3: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-gerrit/54651/ : SUCCESS -- To view, visit https://review.coreboot.org/19998 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib2dcc0895cd5ea149425c1ae01fc1af70e3657e7 Gerrit-PatchSet: 3 Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-HasComments: No

1 0

Change in coreboot[master]: soc: Add AMD Stoney Ridge southbridge code
by Marshall Dawson (Code Review) 31 May '17

31 May '17

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/19722 ) Change subject: soc: Add AMD Stoney Ridge southbridge code ...................................................................... Patch Set 3: (88 comments) Sorry for the huge number of comments. Will need to sync up what I think is still todo vs. Marc's issues he's opened. https://review.coreboot.org/#/c/19722/3//COMMIT_MSG Commit Message: PS3, Line 7: StoneyRidge > Stoney Ridge Done PS3, Line 15: cleanup > clean up Done 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/int I like the cautionary note idea. I believe we're still OK, even with the changes later in the stack. I'll keep an eye out for that though. https://review.coreboot.org/#/c/19722/3/src/soc/amd/common/amd_pci_util.c File src/soc/amd/common/amd_pci_util.c: Line 25: #ifndef __PRE_RAM__ > This whole file is guarded. Why? Just only add it to ramstage-y? I pulled this out in my checkpatch and misc. catch-all patch before noticing the comment. https://review.coreboot.org/#/c/19986 Line 46: void write_pci_int_idx(u8 index, int mode, u8 data) > Do these functions need to be global? todo still. read/write_pci_int_idx should be made static, and prototypes removed from the .h file. Line 85: byte, intr_types[byte], read_pci_int_idx(byte, 1)); > line lengths exceeding 80 in here. printk() strings are fine, but the comme done in checkpatch and misc. fixup https://review.coreboot.org/#/c/19986 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? Hmm, not sure. This file's old counterpart should never be included at the same time. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/bootblock/b… File src/soc/amd/stoneyridge/bootblock/bootblock.c: PS3, Line 18: <device/pci_ids.h> > You aren't using this header? todo? you're probably right. This currently goes away when we switch to C_ENVIRONMENT_BOOTBLOCK https://review.coreboot.org/#/c/19755. PS3, Line 21: 4MB (LPC) ROM access at 0xFFC00000 - 0xFFFFFFFF. > This comment seems stale. todo: determine whether relevant anymore. 48h[4:3] are reserved now. May be OBE with conversion to C_ENVIRONMENT_BOOTBLOCK PS3, Line 34: 0x14, 3 > I thought we already had #defines for these devices? This was picked up in the checkpatch and misc. change https://review.coreboot.org/#/c/19986 PS3, Line 39: 0x48 > magic regs throughout todo: this wasn't picked up however. Change to "LPC_IO_OR_MEM_DECODE_ENABLE". https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/chip.c File src/soc/amd/stoneyridge/chip.c: Line 60: > extra line todo or not. looks like the extra line got dropped in the northbridge-to-soc patch https://review.coreboot.org/#/c/19724/4/src/soc/amd/stoneyridge/chip.c https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/early_setup… File src/soc/amd/stoneyridge/early_setup.c: PS3, Line 103: 0xCD6 > There are #defines for this index/data pair. Moreover, why aren't the commo this gets picked up in the checkpatch and misc. change https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/early_setup… PS3, Line 111: 0x4a > magic literal todo: I didn't want to change this in the same fixup patch because I'd assumed it would affect the build if done properly. 0x4a is part of the 32-bit register at 0x48 (aka LPC_IO_OR_MEM_DECODE_ENABLE). https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/hda.c File src/soc/amd/stoneyridge/hda.c: PS3, Line 21: #include <arch/io.h> : #include <delay.h> : #include <soc/hudson.h> todo: these plus device.h seem no longer used. PS3, Line 26: PCI_DEVICE_ID_AMD_SB900_HDA todo: remove this ID. Line 32: { > Remove the function? todo: agree - remove this and .init hda_audio_ops.init initializer below. PS3, Line 44: 0, > pointer. Not an integer todo: agree - remove this line https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/hudson.c File src/soc/amd/stoneyridge/hudson.c: PS3, Line 65: (0x14 << 3) | 7 > Isn't this PCI_DEVFN(0x14, 7) ?? todo: agree - maybe use "SD_DEVFN" PS3, Line 69: > remove space These three fixed in the checkpatch fixup later https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/hudson.c PS3, Line 126: #if IS_ENABLED(CONFIG_STONEYRIDGE_IMC_FWM) > no #ifs todo: the config option currently removes the support out of Makefile.inc, IIRC, causing a compile error when not enabled. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/ide.c File src/soc/amd/stoneyridge/ide.c: PS3, Line 43: PCI_DEVICE_ID_AMD_SB900_IDE todo: remove this file? ST doesn't have an IDE controller, this defined value (0x780c) doesn't show up in the BKDG. SATA-in-IDE-mode is handled by sata.c. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/imc.c File src/soc/amd/stoneyridge/imc.c: PS3, Line 54: #ifndef __PRE_RAM__ > Why is this guarded? This gets pulled out in the checkpatch and misc. patch https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/imc.c. Line 100: LibAmdMemFill(&(FchParams->Imc.EcStruct), 0, sizeof(FCH_EC), FchParams->StdHeader); > > 80 This gets corrected in the checkpatch and misc. patch https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/imc.c. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/amd… File src/soc/amd/stoneyridge/include/amd_pci_int_defs.h: Line 17: #define AMD_PCI_INT_DEFS_H > All the includes within soc/ directory are namespaced with soc within the i todo, I assume. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/include/amd… File src/soc/amd/stoneyridge/include/amd_pci_int_types.h: PS3, Line 19: [] > [FCH_INT_TABLE_SIZE] todo: I think you're right. I don't recall the size of an array being guaranteed, and if FCH_INIT_TABLE_SIZE is out of sync (i.e. > 0x75 below), this could create a bug for the util code. In fact, the c00/c01 pair is definable through 0x7f. Line 19: const char * intr_types[] = { > This needs to be exposed outside of the module that is currently using it? todo: refactor this and the corresponding amd_pci_util code. Maybe it needs to be exposed in some way... The AMD routing has always been pretty consistent in how the routing is done, but the definitions in the c00/c01 registers may vary. 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 > I think the file will get renamed southbridge.c. I want to rename all the h todo? 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 24: 0x7814 todo: 0x7914 PS3, Line 29: 0x7814 todo: 0x7914 PS3, Line 35: #define SATA_IDE_DEVID 0x7800 : #define AHCI_DEVID_MS 0x7801 : #define AHCI_DEVID_AMD 0x7804 todo: 0x7900, 0x7901, 0x7904 PS3, Line 40: /* OHCI */ : #define OHCI1_DEV 0x12 : #define OHCI1_FUNC 0 : #define OHCI2_DEV 0x13 : #define OHCI2_FUNC 0 : #define OHCI3_DEV 0x16 : #define OHCI3_FUNC 0 : #define OHCI4_DEV 0x14 : #define OHCI4_FUNC 5 : #define OHCI_DEVID 0x7807 : #define OHCI1_DEVFN PCI_DEVFN(OHCI1_DEV,OHCI1_FUNC) : #define OHCI2_DEVFN PCI_DEVFN(OHCI2_DEV,OHCI2_FUNC) : #define OHCI3_DEVFN PCI_DEVFN(OHCI3_DEV,OHCI3_FUNC) : #define OHCI4_DEVFN PCI_DEVFN(OHCI4_DEV,OHCI4_FUNC) todo: remove - no OHCI PS3, Line 56: #define EHCI1_DEV 0x12 : #define EHCI1_FUNC 2 : #define EHCI2_DEV 0x13 : #define EHCI2_FUNC 2 : #define EHCI3_DEV 0x16 : #define EHCI3_FUNC 2 : #define EHCI_DEVID 0x7808 : #define EHCI1_DEVFN PCI_DEVFN(EHCI1_DEV,EHCI1_FUNC) : #define EHCI2_DEVFN PCI_DEVFN(EHCI2_DEV,EHCI2_FUNC) : #define EHCI3_DEVFN PCI_DEVFN(EHCI3_DEV,EHCI3_FUNC) todo: only one EHCI controller. Remove 2 and 3, rename 1. Device ID is 0x7908. PS3, Line 70: 0x780B todo: 0x790b 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? todo: yes remove. no IDE controller. PS3, Line 81: /* HD Audio */ todo: sanity-check all IDs below - odds are they're probably wrong. Update pci_ids.h and use those values here instead of hard-coded? PS3, Line 91: LPC_DEV > Where does LPC_DEV get defined? PCU_DEV is above. todo: fix this; it seems unused and therefore not causing any problems currently. 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? todo: agree, I think these may have been FCH-based ports in Bolton, and we only have GPP off the northbridge, IIRC. 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 todo: agree, and I vote for no leading or trailing underscores at all. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/lpc.c File src/soc/amd/stoneyridge/lpc.c: PS3, Line 110: IORESOURCE_SUBTRACTIVE > subtractive? todo: This could probably be cleaned up. Once upon a time, the PCI bridge in Hudson could be set up to positively decode resources, and anything left over would finally make it out to the LPC device. PS3, Line 114: res->base = 0xff800000; : res->size = 0x00800000; /* 8 MB for flash */ > Hard coding flash size... CONFIG_ROM_SIZE todo: agree PS3, Line 116: IORESOURCE_SUBTRACTIV > subtractive? todo: remove - seems like it would be difficult to have both positive and subtractive for the ROM region. PS3, Line 120: / 1024 > / KiB todo: agree - there are probably additional examples of this. PS3, Line 120: IORESOURCE_SUBTRACTIVE > What makes this SPI_BASE_ADDRESS subtractive? todo: I don't think it would be. Even if it is, let's not configure something over top of it. PS3, Line 156: > extra space Picked up in checkpatch and misc. change https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/lpc.c PS3, Line 156: 512 > space after { and before } todo: ok PS3, Line 157: 0x74 > magic register? todo: change to LPC_ALT_WIDEIO_RANGE_ENABLE PS3, Line 168: 0x44 > more magic? todo: use LPC_IO_PORT_DECODE_ENABLE and LPC_IO_OR_MEM_DECODE_ENABLE below instead of 0x48. PS3, Line 314: /* Set WideIO for as many IOs found (fall through is on purpose) */ : switch (var_num) { : case 3: : pci_write_config16(dev, 0x90, reg_var[2]); : /* fall through */ : case 2: : pci_write_config16(dev, 0x66, reg_var[1]); : /* fall through */ : case 1: : pci_write_config16(dev, 0x64, reg_var[0]); : break; : } > the G EC required earlier setup. This needs cleanup. todo https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/pci.c File src/soc/amd/stoneyridge/pci.c: Line 27: { > Remove init entirely? todo: remove entire file? There shouldn't be a PCI bridge in this device. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/pcie.c File src/soc/amd/stoneyridge/pcie.c: PS3, Line 28: 0 > Remove or use a real pointer todo: remove this entire file? I believe these were in hudson as FCH-based PCIe, which this device doesn't have. In case I'm wrong, the IDs below don't apply anymore. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/reset.c File src/soc/amd/stoneyridge/reset.c: PS3, Line 26: static void set_bios_reset(void) todo: Is this needed? I don't see that BiosRstDet[2:0] or ColdRstDet is ever used. AGESA initiates a couple of resets; maybe it checks these. If so, should we be clearing all BiosRstDet? Setting ColdRstDet? PS3, Line 34: hard_reset > There is a reset patch floating around that will conflict with this: https: todo: Nice catch. It looks like we'll need to change this to do_hard_reset() to match that patch. Maybe we could add a weak hard_reset() here to call do_hard_reset() in case we need to worry about which goes in first. Line 44: outb((0 << 3) | (1 << 2) | (1 << 1), 0xcf9); > More of a warm reset it seems. todo: I agree. Not sure the history that got us here... I'd rename this to soft and add similar one for hard. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/sata.c File src/soc/amd/stoneyridge/sata.c: PS3, Line 38: volatile > Why is this volatile? Use read32 and write32 todo: agree PS3, Line 39: pci_read_config32(dev, AHCI_BASE_ADDRESS_REG) > (void *)(uintptr_t)ALIGN_DOWN(..., 256); todo: I'm ok with the ALIGN_DOWN but it's not as intuitive to me as masking off bits. PS3, Line 65: .scan_bus = 0, > NULL or remove todo: agree PS3, Line 70: PCI_DEVICE_ID_AMD_SB900_SATA, : PCI_DEVICE_ID_AMD_SB900_SATA_AHCI, todo: remove these two. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/sd.c File src/soc/amd/stoneyridge/sd.c: Line 28: stepping = pci_read_config32(dev_find_slot(0, PCI_DEVFN(0x18, 3)), 0xFC); > Do we not have a device macro for PCI_DEVFN(0x18, 3) ? todo: no, I don't think we do yet. todo also: Is this even relevant? It was probably copied from an older device (possibly first introduced in CZ), so should the APU's stepping level be considered when writing the 0xd0 register? Line 30: struct soc_amd_stoneyridge_config *sd_chip = > const todo: sure, sounds good PS3, Line 31: (struct soc_amd_stoneyridge_config *) > cast not needed todo: agree PS3, Line 40: stepping & 0x0000000F) == 0 todo: One of these might be irrelevant. PS3, Line 58: 0 > Set this to NULL or just remove the field initializer. 0 is not a pointer. todo: agree PS3, Line 64: PCI_DEVICE_ID_AMD_YANGTZE_SD todo: This is wrong. Should be 0x7906. (But I thought Marc had SD working...) https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/sm.c File src/soc/amd/stoneyridge/sm.c: PS3, Line 40: * HUDSON enables all USB controllers by default in SMBUS Control. : * HUDSON enables SATA by default in SMBUS Control. I'm not sure what this means. Line 112: { > So there's no resources in this device at all? But it's a pci device? Who's todo: requires more investigation. To me, it looks like the BARs in the BKDG aren't actually BARs, and 0x90 isn't mentioned. Also, SMBus registers can be accessed in the fixed fed8_0a00 (standard) and fed8_0900 (ASF) regions. It may be that these are reserved elsewhere. PS3, Line 134: PCI_DEVICE_ID_AMD_SB900_SM todo: out of date - should be 0x790b. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/smbus.c File src/soc/amd/stoneyridge/smbus.c: Line 19: #include <io.h> > Why is this file so much different than smbus_spd.c? Are there 2 different todo: no, it seems like they should be combined. There have been multiple controllers in some devices, but not in this one. Line 114: u32 address) > same line todo: agree. didn't catch that in my checkpatch cleanup. PS3, Line 145: u32 address, u8 val) todo: this looks like it fits on the line above. PS3, Line 175: alink_ab > It the bus that links the northbridge and the southbridge. Sorry, I'll get a little into the weeds here. The link between the NB and SB (or APU and FCH in the original APUs) was UMI or Unified Media Interface, essentially a x4 PCIe link IIRC. The A and B link was a configurable bridge inside the southbridge. For the most part it's obscured from the user, and shouldn't be confused with a PCI bridge or switch. Line 176: u32 mask, u32 val) > same line todo: agree. PS3, Line 198: u32 mask, u32 val todo: probably fits on the line above. PS3, Line 204: /* rpr 4.2 todo: rpr is very old terminology from the discrete southbridge days. If this is still relevant, update with a passage from the BKDG that can be located. PS3, Line 223: u32 mask, u32 val) todo: probably fits on the line above. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/smbus_spd.c File src/soc/amd/stoneyridge/smbus_spd.c: PS3, Line 19: warning: Porting.h includes an open #pragma pack(1) > This is really bad. todo: This warning has been propagated for years. PS3, Line 45: limit = __rdtsc () + 2000000000 / 10; > We should get monotonic timer going. It's also not clear to me what 2000000 todo: agree, monotonic timer support will help. Timeout seems somewhat arbitrary to me. Line 51: if ((status & 1) == 1) continue; // HostBusy set, keep waiting > and the comment style needs update. todo: >80 fixed in the checkpatch fixup patch, but comment styles not addressed there https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/smbus_spd.c PS3, Line 92: * readspd - Read one or more SPD bytes from a DIMM. : * Start with offset zero and read sequentially. : * Optimization relies on autoincrement to avoid : * sending offset for every byte. : * Reads 128 bytes in 7-8 ms at 400 KHz. : todo: clean up spacing PS3, Line 132: 0xCD6 > We have #defines for these. And I think we probably even have the same help defines used in the same checkpatch cleanup, but I'm not seeing helper functions, at the moment. We should write some if we don't have them. https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/smbus_spd.c PS3, Line 139: 0x2C > Who's backing this bar? These are superio ports? todo: ew, I think this is way out of date. Base and enable are now in PmReg[0]. The base comes up as 0xb00/0xb20 for SMBus and ASF, but I'm not certain who is enabling the decoding. PS3, Line 145: 0xB00 > This should be in a memory map file somewhere so we aren't open coding lite todo: agree. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/smi.c File src/soc/amd/stoneyridge/smi.c: Line 8: > extra new lines? todo: agree PS3, Line 20: hudson > Why are these named with the code name? It makes for big renames later when todo: agree, and there's probably additional scrubbing beyond this. https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/smihandler.c File src/soc/amd/stoneyridge/smihandler.c: Line 113: void southbridge_smi_handler(unsigned int node, smm_state_save_area_t *state_save) > > 80 This gets fixed up in the checkpatch change https://review.coreboot.org/#/c/19986/2/src/soc/amd/stoneyridge/smihandler.c https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/uart.c File src/soc/amd/stoneyridge/uart.c: Line 19: { > assert index <= 1 && >= 0 ? todo: seems reasonable https://review.coreboot.org/#/c/19722/3/src/soc/amd/stoneyridge/usb.c File src/soc/amd/stoneyridge/usb.c: Line 30: { > init contents forthcoming? Or just remove the init() function entirely? todo: remove, nothing in progress. PS3, Line 38: 0 > scan_bus is a function pointer. Please use NULL. todo: remove? -- 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

← Newer
1
2
3
4
5
6
7
8
...
389
Older →

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2017