Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: [WIP] mb/google/zork: add dptc support ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 13: #ifdef DPTC_ENABLE Please split this file's change into its own patch.
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dtpc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/mainboard/google/zork/v... PS3, Line 17: #define SLOW_PPT_LIMIT_MW_0 30000 Are these constants design specific? If so, we shouldn't put them in baseboard.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 2: : /* : * This file is part of the coreboot project. : * : * Copyright (C) 2020 Advanced Micro Devices, Inc. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ remove
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 33: #define DPTC_DGPU_CONTROL 0x10 All these #defines are not indented the same.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 35: #define DPTC_PROFILE_1 0x1 These seem to be duplicated in the header file. Remove that header file.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/dp... PS3, Line 37: Name(DPTI, Buffer(0x07){}) Should we scope this named object?
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/so... File src/soc/amd/picasso/acpi/soc.asl:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/acpi/so... PS3, Line 20: #include "dptc.asl" Please split soc/amd/picasso changes into their own patch.
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/dptc.h:
https://review.coreboot.org/c/coreboot/+/43408/3/src/soc/amd/picasso/include... PS3, Line 3: /* : * This file is part of the coreboot project. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ remove