Benjamin Doron 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:
(38 comments)
https://review.coreboot.org/c/coreboot/+/35523/144//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35523/144//COMMIT_MSG@9 PS144, Line 9: Add initial support for Acer Aspire VN7-572G.
Could you please mention that it's Skylake-U? I always forget whether it's Skylake-U or Skylake-H...
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/Kconfig:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 5: select SYSTEM_TYPE_LAPTOP
Sort these?
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/ac.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 7: EACS
KACS in ec. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 283: (Local1 & 0x08) Yes, which I thought made it little-endian. It's apparently more complicated, but it doesn't matter here anyways.
The Intel 8051, contrary to other Intel processors, expects 16-bit addresses for LJMP and LCALL in big-endian format; however, xCALL instructions store the return address onto the stack in little-endian format.
- https://en.wikipedia.org/wiki/Endianness#History
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 */
These could be moved to ec. […]
I'm keeping the registers on offset 0xE0 here. They number several fields and I don't want to clutter ec.asl. In addition, the paging (or other technique) that they use is specific to battery.asl.
Otherwise, done.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 18: Maps
Aliases?
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 126: Divide
Use ASL 2. […]
This legacy operator touches Local5, which is needed below.
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 29: /* Reserved */
These `Reserved` comments are redundant, if some bits don't have a name they can't be accessed. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 33: Reserved
ETEE in thermal. […]
Done
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 : } : }
Battery stuff, but what does it do? Feel free to add Debug prints to see.
0x40 and 0x41 are probably battery removed/replaced. I probably cannot test that. 0x4C is most probably a battery low warning, but I haven't seen it yet.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 205: Method (_Q50, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : Notify (ADP1, 0x80) // Status Change : } : : Method (_Q51, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : Notify (ADP1, 0x80) // Status Change : }
Charger plugged/unplugged events?
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 215: Method (_Q52, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : Notify (LID0, 0x80) // Status Change : } : : Method (_Q53, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF : { : Notify (LID0, 0x80) // Status Change : }
Lid open/close events?
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 9: ELID
Defined as KLID in ec. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 12: EIDW
Not defined in ec.asl, but it could be moved there.
Done
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))
Not necessary.
It is necessary in the common code, however. I've copied CB:38318 here, I'll remove this file once that is merged.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 37: If (CondRefOf (_SB.MWAK))
Not necessary.
As above.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/thermal.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 8: Offset (0x90), : SCPM, 1, /* Set cooling policy */ : Offset (0x92), : ESSF, 1, : ECTT, 1, : EDTT, 1, : EOSD, 1, /* Trip */ : EVTP, 1, : ECP1, 1, : , 1, : ECP2, 1, : Offset (0xA8), : ES0T, 8, /* Temperature */ : ES1T, 8 /* Temperature */
This could be moved to ec. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 24: Maps
Aliases?
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 33: Offset (0x01), : ETID, 8, : Offset (0xD0), : ESP0, 8, /* Passive temp */ : ESC0, 8, /* Critical temp */ : ESP1, 8, /* Passive temp */ : ESC1, 8 /* Critical temp */
This could be moved to ec. […]
Done
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
I'd drop the commented-out definitions, if you don't use them.
Done. It seems as though some other boards have seen fit to copy the datasheet here, but it doesn't matter.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 74: ChromeOS
Are you going to use ChromeOS?
Not likely, but perhaps vboot.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 78: SandyBridge
Nope.
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 84: amd_reserved
Not an AMD board
True, but the bits exist. I've also seen mention of an upper bank of memory (FSP help text for PchLockDownRtcLock UPD), are they relevant?
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 232: [PchSerialIoIndexUart2] = PchSerialIoDisabled,
I don't know why I thought that LPSS was in-memory, but in any event, it is probably safer to leave […]
INTEL_LPSS_UART_FOR_CONSOLE does select DRIVERS_UART_8250MEM_32. I'll try to test this some time.
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"
I'd recommend checking if the hardware works as intended. […]
Fair point, but I can't test hardware without software.
In any event, microphones using SST come under the NHLT specification, so I'm having it write an NHLT table (DSP blob extracted from vendor's table, between two offsets).
However, it still isn't working. On kernel 5.8.0-rc7, sound doesn't work at all, exactly the same as with the vendor firmware (kernel needed to be patched to allow vendor's violation of the spec - ArrayType=0x1, should be 0xf (vendor defined)). I'll try the kernel Bugzilla.
(Also, for clarity, the microphone is attached to the webcam but wired to the PCH.)
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
Please align these with tabs
Strange, this was aligned in my editor. Done.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 19: register "deep_s3_enable_ac" = "0"
You can drop all zero assignments as the chip struct defaults to zero already. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 53: : register "dptf_enable" = "0"
Can drop
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 56: : register "ProbelessTrace" = "0" : register "EnableLan" = "0"
Can drop
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 65: register "SataPortsDevSlp[0]" = "0" : register "SataPortsDevSlp[1]" = "0" : register "SataPortsDevSlp[2]" = "0"
Can drop
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 71: register "IoBufferOwnership" = "0" : register "EnableTraceHub" = "0" : register "SsicPortEnable" = "0"
Can drop
IoBufferOwnership=0 is a valid setting that indicates that there is no I2S. Otherwise, done.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 75: register "Cio2Enable" = "0" : register "ScsEmmcEnabled" = "0" : register "ScsEmmcHs400Enabled" = "0" : register "ScsSdCardEnabled" = "0" : register "PttSwitch" = "0" : register "SkipExtGfxScan" = "0"
Can drop
PttSwitch=0 is a PTT mode and SkipExtGfxScan=0 is set with PrimaryDisplay. Otherwise, done.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 82: register "Device4Enable" = "0"
Can drop
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 89: register "PmTimerDisabled" = "0"
Can drop
I think this is a genuine setting?
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 3: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB
I'd replace and drop these definitions
Done
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 32: // Mainboard specific
I'd drop this comment
Done
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
Not sure if this is correct
Why not? I copied it from /proc/asound/card0/codec#2.
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])); : }
This is most likely unnecessary for your board
Right, it's for soldered memory. I can see mapping of the pins in the schematics; do you know if it's handled automatically by the hardware or FSP?