Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37483 )
Change subject: mb/gigabyte/ga-h61m-s2pv: Add ga-h61m-ds2v as a variant ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... File src/mainboard/gigabyte/ga-h61m-s2pv/Kconfig:
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 4: ## Copyright (C) 2018-2019 Angel Pons th3fanbus@gmail.com
Yes, I added two boards into this part of the tree this year, but most of the work is not on this fi […]
Well, the copyright notice refers to _this_ file. ASCII art much appreciated ;) I wonder how recognizable a watermelon would be in ASCII ^^
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 34: select NO_UART_ON_SUPERIO if !BOARD_GIGABYTE_GA_H61M_S2PV
I agree. […]
No plans beyond the this-should-have-been-done-from-the-beginning. We could flip it. But all the boards that currently don't select NO_UART_ON_SUPERIO by accident would then select the UART driver which would make it look like we explicitly expect a UART that isn't there. The usual problem of incomplete ports because there was no measure to explicitly state what is on the board. (for most boards the information might still be discoverable on the web, but that's a lot of work to check for it)
Anyway, I meant the ! in that line. I'm used to the NO_UART already that I didn't even notice the double negative. If one reads the line long enough, it's clearly selecting UART for the S2PV which makes sense.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... File src/mainboard/gigabyte/ga-h61m-s2pv/variants/ga-h61m-ds2v/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 17: register "gfx.did" = "{ 0x80000100, 0x80000240, 0x80000410 }" : register "gfx.ndid" = "3"
Autoport adds this. At least ndid matches the length of the did array :D […]
IIRC, it's useful to cycle through the available displays and might matter on laptops that have a special key for it. Never tested if it really works, though.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 22: register "c1_battery" = "1"
Don't give me ideas... […]
Sane defaults sounds very reasonable.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 70: irq 0xf1 = 0x80
About bit 7: […]
Sorry, I was confused by the absence of the PNP_MSC* resources in `superio.c`. Again, I read the `pnp_device.c` and come to the conclusion that they are only necessary for proper warning display.
NB. After all, `pnp_device.c` implements some nice ideas. But together with the pecularities of our allocator, it seems to cause more errors and confusion to register the resources in `superio.c` (see `io` lines below, for instance).
About the bit, sounds like it would be status, hence read-only, doesn't it?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 75: irq 0xf0 = 0x08
This writes the default value, just like the two `io` lines right above. […]
You can't drop the `io` lines. These resources are registered in `superio.c`. The allocator would try to assign them (and likely fail).
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 87: irq 0xc1 = 0x37 : irq 0xcb = 0x00
Yes? […]
Ack
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 89: irq 0xf0 = 0x10 : irq 0xf1 = 0x42 : irq 0xf6 = 0x1c
Yes? I even removed the CMOS battery and went back to 2010 for a while. […]
Ack