Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )
Change subject: mediatek/mt8183: Add SPI GPIO driving setting ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32460/6/src/soc/mediatek/mt8183/gpio.c File src/soc/mediatek/mt8183/gpio.c:
https://review.coreboot.org/#/c/32460/6/src/soc/mediatek/mt8183/gpio.c@170 PS6, Line 170: clrsetbits_le32(reg, 0xf << offset, reg_val << offset); I don't think this works. You would still be running this statement for SPI_PAD1 or bus 2, and in those cases, reg is NULL here.
You could put the code for those special cases directly into the switch-statement and then just return immediately to avoid this problem.
https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c File src/soc/mediatek/mt8183/mt8183.c:
https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c@23 PS2, Line 23: gpio_set_spi_driving();
I'm trying to reduce the code size, please review the latest patch.
Uhh... unless I'm misunderstanding something here, this is a reliability thing, not a speed thing. Increasing your drive strength doesn't make the SPI run faster, but running it with too low drive strength may make it flip a bit somewhere in every one out of a thousand boots. So no, please really do put this in the bootblock. ;)
If you are really worried about code size, one option would be to throw the whole function into the header and make it static inline. That would be ugly, but it would reduce the size to almost nothing (because the compiler can drop all the stuff for busses it doesn't care about).