Change in coreboot[master]: vendor: mediatek: Add mediatek mt8192 dram initialization codes
Attention is currently required from: Xi Chen, Hung-Te Lin, Nico Huber, Martin Roth, Yu-Ping Wu. Julius Werner 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 13:
(8 comments)
File src/vendorcode/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/576ff967_a896bd99 PS13, Line 7: # subdirs-y += mediatek Why is this still commented out?
File src/vendorcode/mediatek/Kconfig:
https://review.coreboot.org/c/coreboot/+/50294/comment/6ef94bea_acca8c4e 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. the MT8192-specific options should go into the mt8192 subdirectory Kconfig). If we expect to also have the same options for future SoCs, they should be prefixed MEDIATEK, not MT8192.
File src/vendorcode/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/f45b0d31_18ac1abf PS13, Line 1: ifeq ($(CONFIG_SOC_MEDIATEK_MT8192),y) This could just be `subdirs-$(CONFIG_SOC_MEDIATEK_MT8192) +=` in the Makefile one level higher.
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 put these in whatever upstream they're maintaining). You can choose to make it BSD or whatever else you want that's compatible with the GPL, of course.
File src/vendorcode/mediatek/mt8192/dramc/emi.c:
https://review.coreboot.org/c/coreboot/+/50294/comment/9f2aa138_4302fb2b 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.
File src/vendorcode/mediatek/mt8192/driver/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/50294/comment/1e2273fd_b1a8f231 PS13, Line 5: romstage-y += uart.c Doesn't coreboot have all these already? Please avoid duplicating drivers between vendorcode and coreboot, everything should be in either one or the other. The vendorcode can call out back to coreboot for these (you can make extra wrapper APIs for them if you want to isolate them a bit more).
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 GPL file where you just stripped off the license header, which is... you know... don't ever do that, please.) Unless I'm missing something in your Makefiles, you're just adding the vendorcode headers to the include path that already contains existing coreboot headers. So the normal version of this file should already be available.
File src/vendorcode/mediatek/mt8192/lib/print.c:
https://review.coreboot.org/c/coreboot/+/50294/comment/30bc222c_6d9db841 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.
-
Julius Werner (Code Review)