Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 3:
(11 comments)
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG@7 PS1, Line 7: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO
Please remove `src/` from the prefix.
Done
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG@10 PS1, Line 10: to I/O register, now.
Now, Super I/O GPIOs can also be controlled directly through access to I/O registers.
Done
https://review.coreboot.org/c/coreboot/+/35849/2/src/device/pnp_device.c File src/device/pnp_device.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/device/pnp_device.c@90 PS2, Line 90: ,
index parameter missing; see pnp_set_iobase. […]
Done through generic pnp resources PNP_IO0, so not needed anymore...
https://review.coreboot.org/c/coreboot/+/35849/2/src/include/device/pnp.h File src/include/device/pnp.h:
https://review.coreboot.org/c/coreboot/+/35849/2/src/include/device/pnp.h@19 PS2, Line 19: ,
index parameter missing
As above
https://review.coreboot.org/c/coreboot/+/35849/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35849/2/src/mainboard/pcengines/apu... PS2, Line 65: device pnp 2e.108 on
don't remove "device pnp 2e.8 off end". […]
Done
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 120: pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT);
use pnp_set_logical_device from device/pnp_ops. […]
Done
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 150: pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT);
use pnp_set_logical_device
Done
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 152: io_base_address = pnp_read_config(dev, 0x61); : io_base_address |= (pnp_read_config(dev, 0x60) << 8);
So, as I understand correctly, that function should only check if uartc and uartd are enabled (and p […]
Reimplemented the check without pnp_read_iobase
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 163: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg | 0x02);
pnp_set_enable flips the bit corresponding to the virtual LDN specified by the struct device *dev pa […]
Used struct device for the VLDN to get it disabled if needed via nct5104d_init
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 111: static void disable_gpio_io_port(struct device *dev)
I wasn't sure if io-mapped gpio won't collide in some cases if uart is set. […]
I have decided to leave this function in place due to the possibility of devicetree + Kconfig (for apu board APU2_PINMUX_*) settings mismatch. I do not feel leaving IO port open for GPIOs when none of them are available, but VLDN 108 still enabled. Let me know what you think.
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 208: { NULL, NCT5104D_GPIO_IO, PNP_IO0, 0x07f8, },
{ NULL, NCT5104D_GPIO_WDT}, shouldn't be removed here; { NULL, NCT5104D_GPIO_IO, PNP_IO0, 0x07f8, } […]
Done