20 comments:
File src/mainboard/asus/a88xm-e/Kconfig:
Patch Set #62, Line 51: config ONBOARD_VGA_IS_PRIMARY
Inherited from F2A85-M, I guess. […]
This means that, by default, a VBIOS for the iGPU needs to be added. If it's not enabled by default, one can still boot without the iGPU VBIOS using a dedicated PCIe graphics card. Also, there are FM2 CPUs without an integrated GPU, so this seems unjustified.
Unless using an external PCIe GPU does not work at all, I don't see why this should be enabled. And if dedicated GPUs were in fact not working, this would deserve a FIXME comment stating so.
File src/mainboard/asus/a88xm-e/OptionsIds.h:
Patch Set #62, Line 32: //#define IDSOPT_IDS_ENABLED TRUE
This stuff gets uncommented when there's a need for debugging. […]
In any case, looks like undefining things isn't necessary (c.f. src/vc/amd/agesa/f15tn/Include/Ids.h), so those can be removed.
File src/mainboard/asus/a88xm-e/acpi/cpstate.asl:
Starting here, you can drop a tab on the rest of the file
File src/mainboard/asus/a88xm-e/acpi/gpe.asl:
Patch Set #66, Line 19: Notify (\_TZ.TZ00, 0x80)
This shouldn't be commented-out
File src/mainboard/asus/a88xm-e/acpi/sata.asl:
CC cbfs/fallback/ramstage.debug […]
Ack
File src/mainboard/asus/a88xm-e/acpi/si.asl:
Code for a System Status Indicator (status LEDs)
Currently, it's just an empty method declaration. Does it do anything at all? I'd try removing it and see if there are any errors.
File src/mainboard/asus/a88xm-e/acpi/sleep.asl:
/* DBGO("\\_WAK\n") */
/* DBGO("From S") */
/* DBGO(Arg0) */
/* DBGO(" to S0\n") */
Some of these "DBG" could be useful for future debugging. […]
... uncommenting each and every line is easier??? Does it even compile?
File src/mainboard/asus/a88xm-e/bootblock.c:
Patch Set #66, Line 14: 0xffc7ffff
Masking of certain bits is usually done with an and-not operation. Using shifts makes it even clearer:
~(7 << 19)
Patch Set #66, Line 15: 0x00100000
Would be clearer to use:
(2 << 19)
And maybe align it with an extra space that compensates the ~ on the previous line
Patch Set #66, Line 19: (~0x80u)
~(1 << 7)
File src/mainboard/asus/a88xm-e/buildOpts.c:
Patch Set #62, Line 50: //#define BLDOPT_REMOVE_UDIMMS_SUPPORT TRUE
could be useful for debugging. […]
You want to debug by removing UDIMM support?
Patch Set #62, Line 143: // Specify the default values for the VRM controlling the VDDNB plane.
Maybe the future explorers will do something good out of it
They will dig... cobwebs? They might as well dig the git history or try rewriting AGESA.
In any case, if not used on Trinity, it makes little sense to have these definitions here
Patch Set #62, Line 184: // #define BLDCFG_SMBUS0_BASE_ADDRESS 0xB00
I don't want to close the road for any future improvement...
One might as well erase all of it because it does not affect the resulting binary. Sounds like several of these values are important, but they might have been defined in other headers already.
Patch Set #62, Line 248: #define DDR400_FREQUENCY 200 ///< DDR 400
Build failed after removal - had to restore.
Ack. Maybe factor them out into their own header
File src/mainboard/asus/a88xm-e/buildOpts.c:
Patch Set #66, Line 74: /* This
Multi-line comments in coreboot should start like this:
/*
* This
File src/mainboard/asus/a88xm-e/devicetree.cb:
io 0x60 = 0x3f0
irq 0x70 = 6
drq 0x74 = 2
Done
No, don't remove the whole device, only the things inside it:
device pnp 2e.0 off end # Floppy
Same thing for the other entries
File src/mainboard/asus/a88xm-e/devicetree.cb:
/* socket 1 - Channel 0 & 1 - 8-bit SPD addresses */
{ {0x00, 0x00}, {0x00, 0x00}, },
Does it build without this?
File src/mainboard/asus/a88xm-e/irq_tables.c:
pirq = (void *)(addr);
v = (u8 *) (addr);
u8 cast cuts the upper 24 bits, while a pirq gets a full value
No, it's casting to an u8 *pointer*, which has the same size of any other pointer. See current patchset for a better way to handle this
File src/mainboard/asus/a88xm-e/irq_tables.c:
Patch Set #66, Line 60: pirq_info = (void *)(&pirq->checksum + 1);
that's just taking the address of "pirq->slots" in a rather fragile way
Ah, I see: this is used to read the bytes for checksumming purposes. Maybe declare it in a scope, and make it const (read-only)?
{
const u8 *const v = (u8 *)(pirq);
for (i = 0; i < pirq->size; i++)
sum += v[i];
}
To view, visit change 30987. To unsubscribe, or for help writing mail filters, visit settings.