Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43133 )
Change subject: baytrail RFC: native replacement for refcode.elf ......................................................................
Patch Set 1: Code-Review+1
(34 comments)
Good work!
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/incl... File src/soc/intel/baytrail/include/soc/modphy.h:
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/incl... PS1, Line 6: static u32 baytrail_modphy_table[] = { See `src/northbridge/intel/sandybridge/raminit_tables.h` and `src/northbridge/intel/sandybridge/raminit_tables.c` as an example. Placing definitions in headers is a bit troublesome, IIRC the reasons are in CB:34876 review messages.
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/incl... PS1, Line 566: #define BAYTRAIL_MODPHY_REVB_OFFSET 0 I'd split the very large array in smaller arrays instead
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... File src/soc/intel/baytrail/refcode.c:
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 9: #if 0 Instead of preprocessor, I'd have two different files and choose which one to build depending on whether the refcode binary was added to the build or not (there's a Kconfig option for that)
Sandy Bridge does something like it to choose between native raminit and MRC raminit, you can take it as an example.
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 90: static u8 mmio_read_8(u32 addr) read8
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 95: static u32 mmio_read_32(u32 addr) read32
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 100: static void mmio_write_32(u32 addr, u32 val) write32
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 121: val2 I'd rename this: or, data
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 121: val1 I'd rename this: and, mask
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 129: static u8 soc_stepping_table[] = const?
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 138: 0xE00F8008 That's something that falls into the PCI config space. 0xe0000000 is the value of MMCONF_BASE_ADDRESS on baytrail. Looks like this is PCI bus:dev.func 0:31.0 (LPC) offset 0x8, which you can replace with:
pci_read_config8(pcidev_on_root(31, 0), 0x8)
Note that the datasheet says this value can be zero, so subtracting one can result in an underflow.
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 143: static void program_modphy_table(u32 *table, u32 entries) See "Message Bus Register Access" section on the datasheet. Then read my suggestions and compare them against what iosf.c does
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 148: mmio_write_32(0xE00000D8, table[1] & 0xffffff00); This is writing MCRX on PCI bus:dev.func 0:0.0 (SoC Transaction Router):
pci_write_config32(pcidev_on_root(0, 0), 0xd8, table[1] & 0xffffff00);
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 149: mmio_write_32(0xE00000D0, 0xf0 : | ((table[1] & 0xff) << 8) : | ((table[0] & 0xff) << 16) : | ((table[4] & 0xff) << 24)); Same story here, but with MCR:
pci_write_config32(pcidev_on_root(0, 0), 0xd0, (0xf << 4) | /* WR Byte Enable */ ((table[1] & 0xff) << 8) | /* Address Offset */ ((table[0] & 0xff) << 16) | /* Message Port */ ((table[4] & 0xff) << 24)); /* Message Opcode */
Also, you can avoid the masks if you define a struct for each line with u8 values.
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 153: tmp = mmio_read_32(0xE00000D4) Now we read MDR:
tmp = pci_read_config32(pcidev_on_root(0, 0), 0xd0);
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 155: table[3] This can be called `value`
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 155: table[2] This should be called `mask` (you can put names if you use a struct instead of a 2D array)
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 157: table[1] Call this `offset`
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 158: table[0] Call this `port`
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 159: table[5] Call this `opcode`
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 171: static void delay_usecs(u64 usecs) udelay
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 181: do_magic_writes gpio_sc_sdcard_workaround
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 182: { 0xfed0c0000 is "IO_BASE_ADDRESS" and in this case is used to poke some GPIO_SCORE registers. I'll explain that later (need to understand this better). In any case, you can add this comment:
/* Workaround for sighting #4684039 */
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 200: do_magic_writes2 ssa_safe_config
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 203: These are message bus operations, all but one are and-or operations.
Also, writes to 0xd0 signals the end of a message bus read or write. However, you've grouped the writes so that the last write to 0xd0 is at the beginning of the next group, which makes it harder to notice that the two writes to 0xd0 of an and-or RMW operation are almost the same (only differ in the read/write opcode)
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 204: mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D0, 0x10030BF0); : tmp = mmio_read_32(0x0E00000D4); : mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D4, (tmp & 0xC0D0D0D0) | 0x1F2F2F2F); : : mmio_write_32(0xE00000D0, 0x11030BF0); Port 0x03, Offset 0x0b = BUNIT_BALIMIT0
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 211: mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D0, 0x100021F0); : tmp = mmio_read_32(0xE00000D4); : mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D4, tmp | 0x80000100); : : mmio_write_32(0xE00000D0, 0x110021F0); Port 0x00, Offset 0x21 = AUNIT_AVCCTL
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 218: mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D0, 0x100060F0); : tmp = mmio_read_32(0xE00000D4); : mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D4, tmp | 0x7FFFFFFF); : : mmio_write_32(0xE00000D0, 0x110060F0); Port 0x00, Offset 0x60 = AUNIT_ACFCACV
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 225: mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D0, 0x100841F0); : tmp = mmio_read_32(0xE00000D4); : mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D4, tmp | 0x7FFFFFFF); : : mmio_write_32(0xE00000D0, 0x110841F0); Port 0x08, Offset 0x41 = CUNIT_ACCESS_CTRL_VIOL
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 232: mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D4, 0x70008); : mmio_write_32(0xE00000D0, 0x110843F0); These three lines are different: instead of a and-or RMW operation, it's just a direct write. So, separate them here.
Port 0x08, Offset 0x43 = CUNIT_SSA_REGIONAL_TRUNKGATE_CTL
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 235: mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D0, 0x100203F0); : tmp = mmio_read_32(0xE00000D4); : mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D4, tmp | 0x110430); : : mmio_write_32(0xE00000D0, 0x110203F0); Port 0x02, Offset 0x03 = TUNIT_CTL
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 242: mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D0, 0x110204F0); : tmp = mmio_read_32(0xE00000D4); : mmio_write_32(0xE00000D8, 0); : mmio_write_32(0xE00000D4, tmp | 0x40010); : mmio_write_32(0xE00000D0, 0x110204F0); Port 0x02, Offset 0x04 = TUNIT_MISC_CTL
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 276: 0xB0 R_PCH_PMC_MTPMC1
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 276: mmio_read_32(0xE00F8044) Another PCI operation!
pci_read_config32(pcidev_on_root(31, 0), 0x44))
https://review.coreboot.org/c/coreboot/+/43133/1/src/soc/intel/baytrail/refc... PS1, Line 280: printk(BIOS_DEBUG, "Polling bit3 of R_PCH_PMC_MTPMC1 = %x\n", val); btw, this looks wrong