Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: soc/amd/picasso: add dptc support ......................................................................
Patch Set 21:
(21 comments)
Chris - just for future reference - all pending comments have to be marked as resolved in order for gerrit to provide the submit option.
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG@12 PS16, Line 12: DPTCi
I think the i here means interface? It would be good to mention that just for context. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/16//COMMIT_MSG@17 PS16, Line 17: TEST=Build. check the setting changed.
Can you please add the generated ACPI code from SSDT to the commit message for reference?
Done
https://review.coreboot.org/c/coreboot/+/43408/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43408/20//COMMIT_MSG@18 PS20, Line 18: Generated ASL code from SSDT
Can you please add the generated SSDT code for the DPTC routine here in the commit message?
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 364: _SB.DPTC(0x0)
Why is this done here?
Done
https://review.coreboot.org/c/coreboot/+/43408/3/src/ec/google/chromeec/acpi... PS3, Line 386: 0x1
Is the argument 0x1 meant to indicate tablet mode is enabled? If so, then this is not correct. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/9/src/mainboard/google/zork/v... PS9, Line 37: POF0
The dptc should used for specific project design, so move it on the board layer. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 26: POF1
I don't understand what you mean by having a dummy function. […]
Done
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 34: POFX
We don't really want to keep dead code which isn't really called/used. […]
Done
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 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. : */
Please remove.
Done
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.
Done
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
Done
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.
Done
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.
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... File src/soc/amd/picasso/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/8/src/soc/amd/picasso/acpi/dp... PS8, Line 10: LEqual
move this method to src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc. […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/chip.h... PS16, Line 160: uint8_t dptc_enable; : uint32_t dptc_fast_ppt_limit; : uint32_t dptc_slow_ppt_limit; : uint32_t dptc_sustained_power_limit;
Can you please move this up along with the other ppt limit params on line 87? […]
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/dptc.h:
PS16:
You don't need a .h file for this. These are used only within a single .c file.
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 136: static void dptci_setup(void)
I think these functions can actually be simplified so that we don't have to create and access fields […]
Done
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 140: acpigen_write_method
If you want serialized, then this should be acpigen_write_method_serialized.
Done