Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 )
Change subject: drivers/apple: Add hybrid graphics driver ......................................................................
Patch Set 8:
(14 comments)
https://review.coreboot.org/#/c/32719/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32719/8//COMMIT_MSG@9 PS8, Line 9: Hybrid graphics driver for Apple MacBook Pro. Would be a good place to give credit to the original author(s).
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... File src/drivers/apple/hybrid_graphics/chip.h:
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... PS8, Line 24: #define HYBRID_GRAPHICS_DEFAULT_GPU HYBRID_GRAPHICS_INTEGRATED Make it a Kconfig?
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... PS8, Line 27: unsigned int `bool`?
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.h:
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 43: #define GMUX_IOSTART 0x700 It would be wise to reserve this for the allocator. For this, you'd set up a `struct device_operations` in enable_dev(). The necessary operation would be `read_resources()` which can add a fixed resource.
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 45: int port Looking at the code it seems limited to 8 bits? So why isn't it a `u8`?
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.c:
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 31: 0x01 It would be nice to give this a name. Looking at the code, it looks much like the `output buffer full` (OBF) of a classic keyboard controller. Whenever this bit is set in GMUX_PORT_WRITE, there is something to read in GMUX_PORT_READ.
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 32: inb(GMUX_IOSTART + GMUX_PORT_READ); Any chance you know what is read (and discarded) here? Some kind of status code, maybe? or is it just for synchronization?
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 34: udelay(100); I think this belongs before the line above it. Currently the sequence is
<-- GMUX_PORT_WRITE <-- GMUX_PORT_READ <-- GMUX_PORT_WRITE delay <-- GMUX_PORT_READ <-- GMUX_PORT_WRITE delay ... <-- GMUX_PORT_READ <-- GMUX_PORT_WRITE delay
Which seems odd at the start.
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 38: return !!i; Shouldn't this be `!(gwr & 0x01)`? i.e. what if the last read succeeded?
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 48: udelay(100); same as above
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 55: return !!i; same as above
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 68: outb((port & 0xff), GMUX_IOSTART + GMUX_PORT_READ); I am very curious if after this another bit in GMUX_PORT_WRITE is set; similar to IBF of a KBC.
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c:
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... PS8, Line 32: /* Don't confuse anyone else and disable the fake device */ Why? Who would get confused?
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c:
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... PS8, Line 30: /* TODO: Use generic device instead of dummy PNP device */ Why is a PNP dev used?