Máté Kukri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43133 )
Change subject: soc/intel/baytrail: Add native refcode replacement ......................................................................
Patch Set 13:
(18 comments)
https://review.coreboot.org/c/coreboot/+/43133/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43133/4//COMMIT_MSG@10 PS4, Line 10: What should be done with all the magic numbers?
They aren't magic if they appear on document #538136 (atom e3800 datasheet) 😄
Done
https://review.coreboot.org/c/coreboot/+/43133/4/src/soc/intel/baytrail/Kcon... File src/soc/intel/baytrail/Kconfig:
https://review.coreboot.org/c/coreboot/+/43133/4/src/soc/intel/baytrail/Kcon... PS4, Line 137: "Disable native high-speed PHY init"
Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/incl... File src/soc/intel/baytrail/include/soc/iosf.h:
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/incl... PS5, Line 97:
align with tabs?
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/incl... PS5, Line 114: 0xa3
0xa6
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/incl... File src/soc/intel/baytrail/include/soc/modphy_table.h:
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/incl... PS5, Line 17: reva
a0
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/incl... PS5, Line 18: revb
b0
Done
https://review.coreboot.org/c/coreboot/+/43133/3/src/soc/intel/baytrail/modp... File src/soc/intel/baytrail/modphy_table.c:
PS3:
The first column is the IOSF-SB port (8-bit value), the second value is the offset (not sure of its […]
Done
https://review.coreboot.org/c/coreboot/+/43133/3/src/soc/intel/baytrail/refc... File src/soc/intel/baytrail/refcode_native.c:
https://review.coreboot.org/c/coreboot/+/43133/3/src/soc/intel/baytrail/refc... PS3, Line 8: static u8 mmio_read_8(u32 addr) : { : return *(u8 *) addr; : } : : static u32 mmio_read_32(u32 addr) : { : return *(u32 *) addr; : } : : static void mmio_write_32(u32 addr, u32 val) : { : *(u32 *) addr = val; : } : : static void mmio_or_32(u32 addr, u32 val) : { : u32 tmp; : : tmp = mmio_read_32(addr); : mmio_write_32(addr, tmp | val); : } : : static void mmio_and_32(u32 addr, u32 val) : { : u32 tmp; : : tmp = mmio_read_32(addr); : mmio_write_32(addr, tmp & val); : } : : static void mmio_andor_32(u32 addr, u32 val1, u32 val2) : { : u32 tmp; : : tmp = mmio_read_32(addr); : mmio_write_32(addr, (tmp & val1) | val2); : } :
Please use `#include <device/mmio.h>` instead. It also draws in <arch/mmio.h> […]
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... File src/soc/intel/baytrail/refcode_native.c:
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 13: static u32 mmio_read32(u32 addr) : { : return *(u32 *) addr; : } : : static void mmio_write32(u32 addr, u32 val) : { : *(u32 *) addr = val; : } : : static void mmio_or32(u32 addr, u32 val) : { : u32 tmp; : : tmp = mmio_read32(addr); : mmio_write32(addr, tmp | val); : } : : static void mmio_and32(u32 addr, u32 val) : { : u32 tmp; : : tmp = mmio_read32(addr); : mmio_write32(addr, tmp & val); : } : : static void mmio_andor32(u32 addr, u32 val1, u32 val2) : { : u32 tmp; : : tmp = mmio_read32(addr); : mmio_write32(addr, (tmp & val1) | val2); : }
please use arch/mmio. […]
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 47: #define IOSF_READ(op_read, port) \
Please fix this, it can bite you
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 64: static inline u64 rdtsc64(void) : { : struct tsc_struct tscval; : : tscval = rdtsc(); : return (u64) tscval.hi << 32 | (u64) tscval.lo; : } : : static void udelay(u64 usecs) : { : u64 needed_ticks, start_ticks; : : needed_ticks = usecs * tsc_freq_mhz(); : start_ticks = rdtsc64(); : while (start_ticks - rdtsc64() < needed_ticks) : ; : }
Please use delay. […]
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 84: 1
This will make more sense if you use shifts: […]
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 85: 4
(1 << 2)
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 86: 0xFFFFFFFD
~(1 << 1)
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 87: 0xFFFFFFF8
~(1 << 3)
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 114: iosf_bunit_write(BUNIT_BALIMIT0, (tmp & 0xC0D0D0D0) | 0x1F2F2F2F);
How about adding and-or functions?
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 137: Replacment
Replacement
Done
https://review.coreboot.org/c/coreboot/+/43133/5/src/soc/intel/baytrail/refc... PS5, Line 144: printk(BIOS_DEBUG, "refcode entry\n");
Given that this is coreboot and not the refcode, you might want a different message?
Done