[coreboot-gerrit] Change in coreboot[master]: soc: Add AMD Stoney Ridge southbridge code
Marshall Dawson (Code Review)
gerrit at coreboot.org
Wed May 31 21:36:38 CEST 2017
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/bootblock.c
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.c
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.c
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_pci_int_defs.h
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_pci_int_types.h
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/hudson.h
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/pci_devs.h
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/smi.h
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 at marcjonesconsulting.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes
More information about the coreboot-gerrit
mailing list