Attention is currently required from: Hung-Te Lin, Xi Chen, Nico Huber, Martin Roth, Paul Menzel, Angel Pons. 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 16:
(3 comments)
File src/vendorcode/mediatek/Kconfig:
https://review.coreboot.org/c/coreboot/+/50294/comment/75c0fbb5_7a6835f8 PS16, Line 3: config DEBUG_DRAM : bool "Output verbose DRAM related debug messages" : default y : help : This option enables additional DRAM related debug messages.
I wonder if we can revise it to something like [...]
That construct will not compile-time eliminate the debug strings from non-debug images, I think for performance you want something where the check is performed on a constant. (You can still move the Kconfig to the SoC directory if you want, of course. That would probably be a bit nicer for menuconfig because the SoC-stuff is not weirdly split apart between different menus.)
File src/vendorcode/mediatek/mt8192/dramc/ANA_init_config.c:
PS13:
Even vendorcode files always need to have an SPDX header […]
I assume that is more about SPDX specifically than about the license in general. I can see that for example the AMD Agesa files have old-style non-SPDX license headers (but with a BSD-style license), and maybe there were concerns about touching that. Maybe Martin or Patrick know more details. I think we should probably change that to only exclude specific subdirectories in vendorcode from the SPDX check if necessary.
But anyway, legally these files don't need an SPDX header specifically but they absolutely need a (GPL-compatible) license. This code is directly linked into GPL files, and distributing the resulting binaries wouldn't be legal without that. The easiest way to fix that would be for MediaTek to just adopt SPDX headers for this code.
File src/vendorcode/mediatek/mt8192/dramc/ANA_init_config.c:
https://review.coreboot.org/c/coreboot/+/50294/comment/3c314ebd_d2d96a38 PS16, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ Just from a practical perspective: while I do love the GPL and want everyone to use it in general, please be aware that if you make this GPL and if later someone else makes any change to this code that you want to sync back to your internally-tracked version for whatever reason, your internal version will become GPL (meaning you need to release sources for any other image you use that code in as well). So if it's not your intention to open source every firmware you use this code in, you may want to make this BSD-3-Clause instead to avoid those problems. (You can also write "GPL-2.0-only OR BSD-3-Clause" to explicitly dual-license it.)
I know that you generally intend this code to just be a one-way street from you to coreboot, but just in case some situation comes up where you'd want to sync something back from here, it would be stupid if licensing details cause that to turn into a huge problem then.