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 16:
(8 comments)
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. DPTC interface(DPTCi) or something on those lines.
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?
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?
Also, I think we should name this something like
uint32_t fast_ppt_limit_tablet_mode; uint32_t slow_ppt_limit_tablet_mode; uint32_t sustained_power_limit_tablet_mode;
This makes it clear that the limits are associated with tablet mode. We could ideally change the other limits on line 87 to add _default or just leave the fields as is.
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.
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 within buffer and do more within C code.
``` enum { ALIB_DPTCI_FUNCTION_ID = 0xc,
SUSTAINED_POWER_LIMIT_PARAM_ID = 0x5, FAST_PPT_LIMIT_PARAM_ID = 0x6, SLOW_PPT_LIMIT_PARAM_ID = 0x7,
DPTC_TOTAL_UPDATE_PARAMS = 3, };
struct dptc_param { uint8_t id; uint32_t value; } __packed;
struct dptc_input { uint16_t size; struct dptc_param params[DPTC_TOTAL_UPDATE_PARAMS]; } __packed;
#define DPTC_INPUTS(_sustained, _fast, _slow) \ { \ .size = sizeof(struct dptc_input), \ .params = { \ { \ .id = SUSTAINED_POWER_LIMIT_PARAM_ID, \ .value = _sustained, \ }, \ { \ .id = FAST_PPT_LIMIT_PARAM_ID, \ .value = _fast, \ }, \ { \ .id = SLOW_PPT_LIMIT_PARAM_ID, \ .value = _slow, \ }, \ }
static void dptc_call_alib(const char *buf_name, uint8_t *buffer, size_t size) { /* Name (buf_name, Buffer(size) {...} */ acpigen_write_name(buf_name); acpigen_write_byte_buffer(buffer, size);
/* _SB.ALIB(0xc, buf_name) */ acpigen_emit_namestring("\_SB.ALIB"); acpigen_write_integer(ALIB_DPTCI_FUNCTION_ID); acpigen_emit_namestring(buf_name); }
static void acipgen_dptci(void) { const config_t *config = config_of_soc();
if (!config->dptc_enable) return;
struct dptc_input default_input = DPTC_INPUTS(config->sustained_power_limit, config->fast_ppt_limit, config->slow_ppt_limit); struct dptc_input tablet_mode_input = DPTC_INPUTS(config->sustained_power_limit_tablet_mode, config->fast_ppt_limit_tablet_mode, config->slow_ppt_limit_tablet_mode);
/* Scope (_SB) */ acpigen_write_scope("\_SB");
/* Method(DPTC, 0, Serialized) */ acpigen_write_method_serialized("DPTC", 0);
/* If (LEqual ("_SB.PCI0.LPCB.EC0.TBMD", 1)) */ acpigen_write_if_lequal_namestr_int("\_SB.PCI0.LPCB.EC0.TBMD", 1);
dptc_call_alib("TABB", (uint8_t *)(void *)&tablet_mode_input, sizeof(tablet_mode_input));
acpigen_pop_len(); /* If */
/* Else */ acpigen_write_else();
dptc_call_alib("DEFB", (uint8_t *)(void *)&default_input, sizeof(default_input));
acpigen_pop_len(); /* Else */
acpigen_pop_len(); /* Method DPTC */ acpigen_pop_len(); /* Scope _SB */ } ```
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.
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 195: dptci_setup(); I think we don't really need this if dptc_enable is not true? Just bail out early:
``` if (!config->dptc_enable) return; ```
https://review.coreboot.org/c/coreboot/+/43408/16/src/soc/amd/picasso/root_c... PS16, Line 205: dptci_update_parameter(DPTC_FAST_PPT_LIMIT, config->dptc_fast_ppt_limit); : dptci_update_parameter(DPTC_SLOW_PPT_LIMIT, config->dptc_slow_ppt_limit); : dptci_update_parameter(DPTC_SUSTAINED_POWER_LIMIT, : config->dptc_sustained_power_limit); Why are we making 3 separate calls to ALIB? As per the spec it looks like ALIB allows multiple parameters to be passed in at the same time?