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 144: Code-Review+1
(40 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...
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?
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.asl
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)
Thanks for the explanation. Done. […]
Isn't the 8051 a 8-bit microcontroller, though?
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.asl
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 18: Maps Aliases?
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 25: /* TODO: Implement paging? */ I'd suggest checking what vendor does in detail, especially if there's some writes to switch between pages.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 115: Local6 = (Local2 / 100) // Warning capacity Not sure if it matters, but I would divide after multiplying to avoid precision loss. Ofc, depends on the raw values.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 126: Divide Use ASL 2.0 operators?
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.
IIRC, one can define multiple fields onto the same OperationRegion with different properties. In that case, having comments for bits that are defined in another Field can be helpful
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 33: Reserved ETEE in thermal.asl
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 177: SMI Does your smihandler handle those?
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.
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?
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?
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.asl
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.
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.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 37: If (CondRefOf (_SB.MWAK)) Not necessary.
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.asl
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 24: Maps Aliases?
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.asl
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.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 74: ChromeOS Are you going to use ChromeOS?
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 78: SandyBridge Nope.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 84: amd_reserved Not an AMD board
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"
The driver would set the pin_cfg, but it isn't working yet anyway. Maybe I'll test with Windows.
I'd recommend checking if the hardware works as intended. Given that most of my hardware isn't new, I can't assume that the hardware is good.
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
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. Feel free to keep those assignments that make things clear for the reader
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 53: : register "dptf_enable" = "0" Can drop
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 56: : register "ProbelessTrace" = "0" : register "EnableLan" = "0" Can drop
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
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
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
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 82: register "Device4Enable" = "0" Can drop
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 89: register "PmTimerDisabled" = "0" Can drop
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
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 32: // Mainboard specific I'd drop this comment
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
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