Attention is currently required from: Xi Chen, Hung-Te Lin, Nico Huber, Martin Roth, Paul Menzel, Julius Werner, Yu-Ping Wu. Xi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50294 )
Change subject: vendor: mediatek: Add mediatek mt8192 dram initialization codes ......................................................................
Patch Set 16:
(17 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/50294/comment/282e15a9_bf695bf7 PS5, Line 11:
However, in case Mediatek maintains a public Git repository, we could also integrated that as a su […]
Done
Commit Message:
https://review.coreboot.org/c/coreboot/+/50294/comment/378c9437_4672d5c6 PS9, Line 9: Add the DRAM initialization code based on Mediatek reference implementation.
This is the DRAM calibration code from the reference […]
Done
https://review.coreboot.org/c/coreboot/+/50294/comment/ce8919bf_9f2ae4c8 PS9, Line 11: Mediatek internally maintains the DRAM initialization code, following : different coding style. : : To prevent maintaining a different branch for coreboot : (which may lead to typo or errors which switching between different coding : style), we want to directly use the reference implementation as vendor code.
The ETT is a standalone library, used by different boot loaders […]
Done
Patchset:
PS13:
Please also update the file `MAINTAINERS`.
Add mt8192 maintainers on MEDIATEK MT8192 SOC.
File src/vendorcode/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/88b4148e_1b6c5927 PS8, Line 7: #
If that's the case, I'd prefer adding a new Kconfig, say MEDIATEK_VENDOR_DRAM_CALIBRATION or MEDIATE […]
Done
File src/vendorcode/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/d5859fbf_e3246a50 PS13, Line 7: # subdirs-y += mediatek
Why is this still commented out?
move this to CB:51125.
File src/vendorcode/mediatek/Kconfig:
https://review.coreboot.org/c/coreboot/+/50294/comment/bb032b7b_ba8ad279 PS9, Line 9: config MT8192_DRAM_EMCP : bool : default y : help : The eMCP platform should select this option to run at different DRAM : frequencies.
no used, will remove.
Done
File src/vendorcode/mediatek/Kconfig:
https://review.coreboot.org/c/coreboot/+/50294/comment/6a981c66_aed9b061 PS13, Line 9: config MT8192_DRAM_DVFS
Please try to design this from the start so it will make sense when more SoCs are added (i.e. […]
It's a common option for mediatek, change to MEDIATEK_DRAM_DVFS.
File src/vendorcode/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/1f82741f_f5e4fd1c PS9, Line 7: ramstage-y += dpm.c
will move to soc folder.
Done
https://review.coreboot.org/c/coreboot/+/50294/comment/4fb1126d_7f01b51d PS9, Line 9: BL31_MAKEARGS += PLAT=mt8192
yes, no need, will remove.
Done
File src/vendorcode/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/ae5a6d01_30d3a8be PS13, Line 1: ifeq ($(CONFIG_SOC_MEDIATEK_MT8192),y)
This could just be `subdirs-$(CONFIG_SOC_MEDIATEK_MT8192) +=` in the Makefile one level higher.
Ack
File src/vendorcode/mediatek/mt8192/dpm.c:
https://review.coreboot.org/c/coreboot/+/50294/comment/ab4e9177_5c69b1cd PS9, Line 1: License
will move to soc folder.
Done
File src/vendorcode/mediatek/mt8192/dramc/ANA_init_config.c:
PS13:
Even vendorcode files always need to have an SPDX header (it would be great if MediaTek could just p […]
Add SPDX license.
File src/vendorcode/mediatek/mt8192/dramc/emi.c:
https://review.coreboot.org/c/coreboot/+/50294/comment/956545f9_c949d07d PS13, Line 5: Without : * the prior written permission of MediaTek inc. and/or its licensors, any : * reproduction, modification, use or disclosure of MediaTek Software, and : * information contained herein, in whole or in part, shall be strictly : * prohibited.
This doesn't work. It still needs to be open-source code.
change to SPDX license.
File src/vendorcode/mediatek/mt8192/driver/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/20c36ba2_a4d095a3 PS13, Line 5: romstage-y += uart.c
Doesn't coreboot have all these already? Please avoid duplicating drivers between vendorcode and cor […]
already remove them.
File src/vendorcode/mediatek/mt8192/include/stdint.h:
PS13:
Why is this file duplicated here? (Not to mention the fact that this is clearly an original coreboot […]
Already remove it.
File src/vendorcode/mediatek/mt8192/lib/print.c:
https://review.coreboot.org/c/coreboot/+/50294/comment/f98d132a_444ce9e2 PS13, Line 101: static int vprint(const char *fmt, va_list ap)
This too is something we really don't want to have a duplicate of in the codebase.
already remove this function.