Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32789 )
Change subject: soc/intel/icelake: Make use of gpio_pm_configure() ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.h File src/soc/intel/icelake/chip.h:
https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.h@278 PS6, Line 278: TOTAL_GPIO_COMM Can we add an enum or macro in some header file for each community? Then it can be used in gpio.c for the communities: icl_communities[TOTAL_GPIO_COMM] = { [COMM_0] = { ... }, [COMM_1] = { ... }, };
And same can be used in devicetree: register "gpio_pm[COMM_0]" = "..."
That will ensure if there is any change in icl_communities, mainboards do not need to be updated.
https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c File src/soc/intel/icelake/chip.c:
https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@113 PS6, Line 113: PCH_DEV_SLOT_LPC Why LPC?
https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@120 PS6, Line 120: config->gpio_pm[i].config Can we treat 0 as special value or maybe have another flag that indicates mainboard has set these values? I am just concerned that some mainboard might forget to set this and end up with no PM enabled for any community. Thoughts?
https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@121 PS6, Line 121: mask[i] = ~value[i]; I don't think this is correct. It will never reset a bit that mainboard requests if it is already set in the config register.
https://review.coreboot.org/#/c/32789/6/src/soc/intel/icelake/chip.c@142 PS6, Line 142: TODO(subrata): Need to implement 4us IRQ width recommendation. Where do you plan to implement this?