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:
(3 comments)
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); : } : :
We have intentionally dropped the guards, and they are inlined for better code density.
Ack
https://review.coreboot.org/c/coreboot/+/37813/1/src/soc/amd/common/block/in... PS1, Line 387: /* Old GPIO configuration registers */
Agree, for the old GPIO block at 0x100 it may be a more appropriate way.
But are you planning on supporting bootblock GPIO programming on old platforms? It has not been necessary so far, and most platforms can wait some milliseconds until romstage is reached and than programming GPIO.
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); : }
My goal was to unify the GPIO configuration for mainboards. […]
Older families that implement this GPIO and are not SOC included will implement GPIO through AGESA, normally at romstage and sometimes at a later stage. And yes, in this situation you have to be an expert to know the bytes to be placed on the table that is fed to AGESA. I would really prefer to not have these functions available, it opens a big can of worms with possibility of mistakes done by those that don't fully understand (final client/user) what they are doing.