Patch set 144:Code-Review +1
40 comments:
Patch Set #144, 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...
File src/mainboard/acer/aspire_vn7_572g/Kconfig:
Patch Set #144, Line 5: select SYSTEM_TYPE_LAPTOP
Sort these?
File src/mainboard/acer/aspire_vn7_572g/acpi/ac.asl:
KACS in ec.asl
File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
Patch Set #122, Line 283: (Local1 & 0x08)
Thanks for the explanation. Done. […]
Isn't the 8051 a 8-bit microcontroller, though?
File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
Patch Set #144, Line 4: /* EC Registers */
These could be moved to ec.asl
Aliases?
Patch Set #144, Line 25: /* TODO: Implement paging? */
I'd suggest checking what vendor does in detail, especially if there's some writes to switch between pages.
Patch Set #144, 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.
Patch Set #144, Line 126: Divide
Use ASL 2.0 operators?
File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
Patch Set #144, 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
Patch Set #144, Line 33: Reserved
ETEE in thermal.asl
Does your smihandler handle those?
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.
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?
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?
File src/mainboard/acer/aspire_vn7_572g/acpi/mainboard.asl:
Defined as KLID in ec.asl
Not defined in ec.asl, but it could be moved there.
File src/mainboard/acer/aspire_vn7_572g/acpi/platform.asl:
Patch Set #144, Line 27: If (CondRefOf (\_SB.MPTS))
Not necessary.
Patch Set #144, Line 37: If (CondRefOf (\_SB.MWAK))
Not necessary.
File src/mainboard/acer/aspire_vn7_572g/acpi/thermal.asl:
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
Aliases?
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
File src/mainboard/acer/aspire_vn7_572g/cmos.layout:
Patch Set #144, Line 7: #0 8 r 0 seconds
I'd drop the commented-out definitions, if you don't use them.
Patch Set #144, Line 74: ChromeOS
Are you going to use ChromeOS?
Patch Set #144, Line 78: SandyBridge
Nope.
Patch Set #144, Line 84: amd_reserved
Not an AMD board
File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
Patch Set #119, 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.
File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
Patch Set #144, Line 12: register "gpu_pp_down_delay_ms" = "50" # T10
Please align these with tabs
Patch Set #144, 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
register "dptf_enable" = "0"
Can drop
register "ProbelessTrace" = "0"
register "EnableLan" = "0"
Can drop
register "SataPortsDevSlp[0]" = "0"
register "SataPortsDevSlp[1]" = "0"
register "SataPortsDevSlp[2]" = "0"
Can drop
register "IoBufferOwnership" = "0"
register "EnableTraceHub" = "0"
register "SsicPortEnable" = "0"
Can drop
register "Cio2Enable" = "0"
register "ScsEmmcEnabled" = "0"
register "ScsEmmcHs400Enabled" = "0"
register "ScsSdCardEnabled" = "0"
register "PttSwitch" = "0"
register "SkipExtGfxScan" = "0"
Can drop
Patch Set #144, Line 82: register "Device4Enable" = "0"
Can drop
Patch Set #144, Line 89: register "PmTimerDisabled" = "0"
Can drop
File src/mainboard/acer/aspire_vn7_572g/dsdt.asl:
#define BRIGHTNESS_UP \_SB.PCI0.GFX0.INCB
#define BRIGHTNESS_DOWN \_SB.PCI0.GFX0.DECB
I'd replace and drop these definitions
Patch Set #144, Line 32: // Mainboard specific
I'd drop this comment
File src/mainboard/acer/aspire_vn7_572g/hda_verb.c:
Patch Set #144, Line 70: 0x80860101
Not sure if this is correct
File src/mainboard/acer/aspire_vn7_572g/romstage.c:
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
To view, visit change 35523. To unsubscribe, or for help writing mail filters, visit settings.