Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37813 )
Change subject: amdblocks/acpimmio: add missing MMIO functions ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37813/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37813/1//COMMIT_MSG@9 PS1, Line 9: new I don't agree on adding new GPIO. There are functions already for them.
https://review.coreboot.org/c/coreboot/+/37813/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio.h:
https://review.coreboot.org/c/coreboot/+/37813/1/src/soc/amd/common/block/in... PS1, Line 217: static inline uint8_t pm2_read8(uint8_t reg) : { : return read8((void *)(ACPIMMIO_PMIO2_BASE + reg)); : } : : static inline uint16_t pm2_read16(uint8_t reg) : { : return read16((void *)(ACPIMMIO_PMIO2_BASE + reg)); : } : : static inline uint32_t pm2_read32(uint8_t reg) : { : return read32((void *)(ACPIMMIO_PMIO2_BASE + reg)); : } : : static inline void pm2_write8(uint8_t reg, uint8_t value) : { : write8((void *)(ACPIMMIO_PMIO2_BASE + reg), value); : } : : static inline void pm2_write16(uint8_t reg, uint16_t value) : { : write16((void *)(ACPIMMIO_PMIO2_BASE + reg), value); : } : : static inline void pm2_write32(uint8_t reg, uint32_t value) : { : write32((void *)(ACPIMMIO_PMIO2_BASE + reg), value); : } : : These functions should be protected by SUPPORTS_ACPIMMIO_PMIO2_BASE. Even better, declare them at soc/amd/common/block/acpimmio/mmio_util.c and only leave the prototypes here, still protected by SUPPORTS_ACPIMMIO_PMIO2_BASE (and don't use inline or static).
https://review.coreboot.org/c/coreboot/+/37813/1/src/soc/amd/common/block/in... PS1, Line 387: /* Old GPIO configuration registers */ new SOC don't implement these registers, old SOC manipulated them through AGESA. What is the point of implementing these functions, unless you plan to support old SOC GPIO without using AGESA (for clarity and speed?).
https://review.coreboot.org/c/coreboot/+/37813/1/src/soc/amd/common/block/in... PS1, Line 417: : /* New GPIO banks configuration registers */ : /* GPIO bank 0 */ : static inline uint8_t gpio0_read8(uint8_t reg) : { : return read8((void *)(ACPIMMIO_GPIO0_BASE + reg)); : } : : static inline uint16_t gpio0_read16(uint8_t reg) : { : return read16((void *)(ACPIMMIO_GPIO0_BASE + reg)); : } : : static inline uint32_t gpio0_read32(uint8_t reg) : { : return read32((void *)(ACPIMMIO_GPIO0_BASE + reg)); : } : : static inline void gpio0_write8(uint8_t reg, uint8_t value) : { : write8((void *)(ACPIMMIO_GPIO0_BASE + reg), value); : } : : static inline void gpio0_write16(uint8_t reg, uint16_t value) : { : write16((void *)(ACPIMMIO_GPIO0_BASE + reg), value); : } : : static inline void gpio0_write32(uint8_t reg, uint32_t value) : { : write32((void *)(ACPIMMIO_GPIO0_BASE + reg), value); : } : : /* GPIO bank 1 */ : static inline uint8_t gpio1_read8(uint8_t reg) : { : return read8((void *)(ACPIMMIO_GPIO1_BASE + reg)); : } : : static inline uint16_t gpio1_read16(uint8_t reg) : { : return read16((void *)(ACPIMMIO_GPIO1_BASE + reg)); : } : : static inline uint32_t gpio1_read32(uint8_t reg) : { : return read32((void *)(ACPIMMIO_GPIO1_BASE + reg)); : } : : static inline void gpio1_write8(uint8_t reg, uint8_t value) : { : write8((void *)(ACPIMMIO_GPIO1_BASE + reg), value); : } : : static inline void gpio1_write16(uint8_t reg, uint16_t value) : { : write16((void *)(ACPIMMIO_GPIO1_BASE + reg), value); : } : : static inline void gpio1_write32(uint8_t reg, uint32_t value) : { : write32((void *)(ACPIMMIO_GPIO1_BASE + reg), value); : } : : /* GPIO bank 2 */ : static inline uint8_t gpio2_read8(uint8_t reg) : { : return read8((void *)(ACPIMMIO_GPIO2_BASE + reg)); : } : : static inline uint16_t gpio2_read16(uint8_t reg) : { : return read16((void *)(ACPIMMIO_GPIO2_BASE + reg)); : } : : static inline uint32_t gpio2_read32(uint8_t reg) : { : return read32((void *)(ACPIMMIO_GPIO2_BASE + reg)); : } : : static inline void gpio2_write8(uint8_t reg, uint8_t value) : { : write8((void *)(ACPIMMIO_GPIO2_BASE + reg), value); : } : : static inline void gpio2_write16(uint8_t reg, uint16_t value) : { : write16((void *)(ACPIMMIO_GPIO2_BASE + reg), value); : } : : static inline void gpio2_write32(uint8_t reg, uint32_t value) : { : write32((void *)(ACPIMMIO_GPIO2_BASE + reg), value); : } I don't see value on these. GPIO configuration is quite complicated, accomplished by soc/amd/common/block/gpio_banks/gpio.c using macros and tables. At most I would leave the read functions, I don't want anyone messing with these registers without knowing what they are doing... but it would be better to fully hide, thus I would also remove the read functions.