Kyösti Mälkki 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/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. […]
We have intentionally dropped the guards, and they are inlined for better code density.
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. […]
Keeping VBOOT in mind, bootblock will not be allowed to call AGESA, specially the blobbed one.
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. […]
I kind of agree about the hiding but we are not there yet and things are open-coded currently. I believe we will identify some of these direct GPIO manipulations will translate into composite enable 48MHz clock function calls.
https://review.coreboot.org/c/coreboot/+/37813/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37813/1/src/soc/amd/common/block/in... PS1, Line 51: #define ACPIMMIO_GPIO_BASE_100 0xfed80100 How was it called int the specs?
_LEGACY_GPIO_ ?