Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35523 )
Change subject: mb/acer: Add Acer Aspire VN7-572G ......................................................................
Patch Set 145: Code-Review+1
(20 comments)
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 4: /* EC Registers */
I'm keeping the registers on offset 0xE0 here. […]
Sounds good
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 126: Divide
This legacy operator touches Local5, which is needed below.
Ack. That's the remainder, I think?
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 135: Arg1 any idea of what each bit means?
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 261: Local0 = EB0A This was using EB0S before, was it wrong?
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 182: Method (_Q40, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : Notify (BAT0, 0x81) // Information Change : } : : Method (_Q41, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : Notify (BAT0, 0x81) // Information Change : } : : Method (_Q48, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : Notify (BAT0, 0x80) // Status Change : } : : Method (_Q4C, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : If (B0ST) : { : Notify (BAT0, 0x80) // Status Change : } : }
0x40 and 0x41 are probably battery removed/replaced. I probably cannot test that. […]
Ack
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 251: // _Qxx: EC Query, xx=0x00-0xFF I'd drop these comments for known (or just all) queries
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 27: If (CondRefOf (_SB.MPTS))
It is necessary in the common code, however. […]
Ah, then I'd rebase CB:38318 so that it can be applied directly on master
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/cmos.layout:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 7: #0 8 r 0 seconds
Done. […]
I'm used to seeing lots of copypasta in coreboot, so I often poke people about it
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 74: ChromeOS
Not likely, but perhaps vboot.
Ack
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 84: amd_reserved
True, but the bits exist. […]
I'd just drop it.
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/cmos.layout:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 7: #0 112 r 0 lower_reserved This one wasn't commented out:
0 120 r 0 reserved_memory
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 8: #112 264 r 0 unused These `unused` commented-out entries are just extra maintenance burden
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 36: #1024 1024 r 0 upper_reserved I don't think it's reserved. IIRC, handling of the upper half of CMOS in coreboot isn't working very well, but I never tried it myself.
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... PS119, Line 68: # TODO: register "DspEndpointDmic" = "3"
Fair point, but I can't test hardware without software. […]
Ack. Is it possible to change that ArrayType value?
Or, you might need to have some quirk handling implemented in the kernel. coreboot will have to behave the same as vendor in order for the quirk to be triggered.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 12: register "gpu_pp_down_delay_ms" = "50" # T10
Strange, this was aligned in my editor. Done.
Tabs are 8 spaces wide in coreboot. If we want to allow using them for other alignment purposes, we should align everything with spaces instead (which is annoying)
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 89: register "PmTimerDisabled" = "0"
I think this is a genuine setting?
It defaults to zero already
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/gpio.h:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 259: PAD_CFG_GPIO_BIDIRECT(GPP_E22, 0, NONE, DEEP, LEVEL, ACPI), Nico commented on it:
What is this useful for? All the mentioned boards have probably copy-pasta entries that nobody reasoned about. Adding a macro might make it look like the entries are intentional and yet something more to mantain... I don't think we should add any entry that doesn't make sense in the context of the coreboot ports.
If there's a legitimate use-case for this macro, I'd explain why it is needed in that change.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 70: 0x80860101
Why not? I copied it from /proc/asound/card0/codec#2.
Acer uses a different subsystem ID. Not a big deal though
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/romstage.c:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 8: static void mainboard_fill_dq_map_data(void *dq_map_ch0, void *dq_map_ch1) : { : /* DQ byte map */ : const u8 dq_map[2][12] = { : { 0x0F, 0xF0, 0x00, 0xF0, 0x0F, 0xF0, : 0x0F, 0x00, 0xFF, 0x00, 0xFF, 0x00 }, : { 0x33, 0xCC, 0x00, 0xCC, 0x33, 0xCC, : 0x33, 0x00, 0xFF, 0x00, 0xFF, 0x00 } }; : memcpy(dq_map_ch0, dq_map[0], sizeof(dq_map[0])); : memcpy(dq_map_ch1, dq_map[1], sizeof(dq_map[1])); : } : : static void mainboard_fill_dqs_map_data(void *dqs_map_ch0, void *dqs_map_ch1) : { : /* DQS CPU<>DRAM map */ : const u8 dqs_map[2][8] = { : { 0, 1, 3, 2, 4, 5, 6, 7 }, : { 1, 0, 4, 5, 2, 3, 6, 7 } }; : memcpy(dqs_map_ch0, dqs_map[0], sizeof(dqs_map[0])); : memcpy(dqs_map_ch1, dqs_map[1], sizeof(dqs_map[1])); : }
Right, it's for soldered memory. […]
I think it's only used for LPDDR. For regular DDR, the only routing-related setting is DqPinsInterleaved
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/romstage.c:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 34: TRUE true (in lowercase)