Patch set 145:Code-Review +1
20 comments:
File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
Patch Set #144, Line 4: /* EC Registers */
I'm keeping the registers on offset 0xE0 here. […]
Sounds good
Patch Set #144, Line 126: Divide
This legacy operator touches Local5, which is needed below.
Ack. That's the remainder, I think?
File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
Patch Set #145, Line 135: Arg1
any idea of what each bit means?
Patch Set #145, Line 261: Local0 = EB0A
This was using EB0S before, was it wrong?
File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
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
File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
Patch Set #145, Line 251: // _Qxx: EC Query, xx=0x00-0xFF
I'd drop these comments for known (or just all) queries
File src/mainboard/acer/aspire_vn7_572g/acpi/platform.asl:
Patch Set #144, 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
File src/mainboard/acer/aspire_vn7_572g/cmos.layout:
Patch Set #144, 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
Patch Set #144, Line 74: ChromeOS
Not likely, but perhaps vboot.
Ack
Patch Set #144, Line 84: amd_reserved
True, but the bits exist. […]
I'd just drop it.
File src/mainboard/acer/aspire_vn7_572g/cmos.layout:
Patch Set #145, Line 7: #0 112 r 0 lower_reserved
This one wasn't commented out:
0 120 r 0 reserved_memory
Patch Set #145, Line 8: #112 264 r 0 unused
These `unused` commented-out entries are just extra maintenance burden
Patch Set #145, 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.
File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
Patch Set #119, 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.
File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
Patch Set #144, 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)
Patch Set #144, Line 89: register "PmTimerDisabled" = "0"
I think this is a genuine setting?
It defaults to zero already
File src/mainboard/acer/aspire_vn7_572g/gpio.h:
Patch Set #145, 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.
File src/mainboard/acer/aspire_vn7_572g/hda_verb.c:
Patch Set #144, 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
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]));
}
Right, it's for soldered memory. […]
I think it's only used for LPDDR. For regular DDR, the only routing-related setting is DqPinsInterleaved
File src/mainboard/acer/aspire_vn7_572g/romstage.c:
true (in lowercase)
To view, visit change 35523. To unsubscribe, or for help writing mail filters, visit settings.