Angel Pons 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:
(11 comments)
I think some of the comments also apply to the other variants already in the tree. Since the floor hasn't collapsed yet, I'm pretty sure I still have the other boards buried somewhere inside this room. If I find them, I'll boot-test the entire family.
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
Did you add new, creative work, this year?
Yes, I added two boards into this part of the tree this year, but most of the work is not on this file. I guess Gigabyte designers were rather lazy when designing these boards, and made them very similar. Want me to add some ASCII art of watermelons to make up for it?
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
Would be much easier to read without the inversion.
I agree. Handling negative Kconfig symbols (that is, symbols that represent the *absence* of something) is cumbersome. So, do you have any plans to flip that symbol?
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"
Is this useful?
Autoport adds this. At least ndid matches the length of the did array :D
In any case, this does not seem particularly useful. The real question is, does *any* board need these? I think it's just dead code in 90% of all snb/ivb boards
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 22: register "c1_battery" = "1"
Will the *battery values ever be used?
Don't give me ideas... I don't think so, but I have not checked what would happen if they are omitted.
In any case, only a few boards should need special values here, so why not have sane defaults in the CPU code instead?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 44: device pci 16.0 on end # Management Engine Interface 1 Can drop some unused devices
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 70: irq 0xf1 = 0x80
Does this work? I see no entry for this in the respective `superio.c`, […]
About bit 7: This bit is set to 1 when AVCC3 is on at the previous AC power failure whereas 0 when AVCC3 is off.
I guess it's not needed.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 75: irq 0xf0 = 0x08
Does this work? I see no entry for this in the respective `superio.c`, […]
This writes the default value, just like the two `io` lines right above. Do I drop only this, or the three lines?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 79: irq 0x25 = 0x40 : irq 0x26 = 0xf7 : irq 0x27 = 0x10 : irq 0x2c = 0x80
Does this work?
Yes? I even removed the CMOS battery and went back to 2010 for a while.
These are muxing GPIOs and such, so without looking at the schematics I'd rather not risk it.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 87: irq 0xc1 = 0x37 : irq 0xcb = 0x00
Does this work?
Yes?
Is it needed? Maybe not, but I'm not sure if I have schematics to check what these GPIOs do.
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
Does this work? I see no entries for these in the respective `superio.c`, […]
Yes? I even removed the CMOS battery and went back to 2010 for a while.
0xf0: SMI# control register 1. Enables the Environmental Controller IRQs to generate SMIs. 0xf1: SMI# control register 2. Sets level trigger and enables the WDT IRQs to generate SMIs. 0xf6: Alert beep pin location.
Maybe I don't need these when I don't care the slightest about SMIs, but played safe and mirrored vendor because this is the GPIO LDN.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... File src/mainboard/gigabyte/ga-h61m-s2pv/variants/ga-h61m-ds2v/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 22: 0x0000000f, /* Number of 4 dword sets */
Why write the number in hex? makes review harder.
Ask autoport about it, maybe that pile of Go code likes the alignment that 8-nibble hex quantities provide? I'm pretty sure there's at least two dozen boards in the tree which do the exact same thing.