Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33993 )
Change subject: mainboard/amd: Add padmelon board code ......................................................................
Patch Set 15:
(27 comments)
https://review.coreboot.org/c/coreboot/+/33993/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33993/15//COMMIT_MSG@19 PS15, Line 19: SOC SOCs
https://review.coreboot.org/c/coreboot/+/33993/15//COMMIT_MSG@20 PS15, Line 20: simi;ar similar
https://review.coreboot.org/c/coreboot/+/33993/15//COMMIT_MSG@22 PS15, Line 22: Marc Marc Jones
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/BiosCallOuts.h:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 31: fan runs Either "4-wire fans run at" or "a 4-wire fan runs at". Same for the 2nd half of the sentence.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/Kconfig:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 18: config HAVE_S3_SUPPORT Put this under the mainboard part number?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 22: Select it to change GPIO used for wake. What? This help makes no sense. HAVE_S3_SUPPORT should be about whether the board supports S3 or not.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 61: Select a bit within LPC_IO_PORT_DECODE_ENABLE that will enable : access to IO port HWM_PORT, which is programmed at bootblock. I still have no idea what this does or why it should be a Kconfig option. Can this just be hardcoded in a header file?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 64: config HWM_PORT Again, does this need to be a Kconfig option? There's no prompt, why not just set it in a header file.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 68: HWM Maybe lead with what HWM stands for and what it's used for.
The Hardware monitor is used for xxxx
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/acpi/gpe.asl:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 17: You might want to verify the methods in this file. These look like they're left over from other platforms.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/acpi/routing.asl:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 17: /* : DefinitionBlock ("DSDT.AML","DSDT",0x01,"XXXXXX","XXXXXXXX",0x00010001 : ) : { : #include "routing.asl" : } : */ : Remove this block?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 49: #define SUPERIO_PNP_BASE 0x4E It seems a little odd to define this inside an ASL file. Is this value not used anywhere else? Can this be set by a #define in a header file that gets used here?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 52: Device(SUPERIO_DEV) { Why is this in the mainboard instead of the SIO's directory?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/acpi/usb_oc.asl:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 17: /* simple name description */ : /* : DefinitionBlock ("DSDT.AML","DSDT",0x01,"XXXXXX","XXXXXXXX",0x00010001 : ) : { : #include "usb.asl" : } : */ remove this block?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 38: /* USB Overcurrent GPEs */ : /* TODO: Update for Gardenia */ Just update this? Why would you leave a TODO for Gardenia in the padmelon code? Either remove the comment, or update it.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 1: /* You can remove the license header - there's really nothing to license.
If you want to keep the license for some reason, add the line that says that this file is a part of the coreboot project please.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/board_info.txt:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 2: Can't we fill out any other fields?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/bootblock/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 121: /*DESCRIPTOR_TERMINATE_LIST, */ Remove this comment?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 140: AzaliaCodecAlc286Table Is this the correct codec for padmelon?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 162: { 0x10ec0286, AzaliaCodecAlc286Table}, : { 0x10ec0288, AzaliaCodecAlc286Table}, : { 0x10ec0888, AzaliaCodecAlc286Table}, We use the same table for 3 different codecs?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 35: /* Remove this section if HW handshake is not needed */ Is the HW handshake needed? If so, remove the comment. Otherwise, remove the block.
Looking at the comment below, it seems like they're needed for some reason, but I'm not sure why that would be.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 27: #register "uma_size" = "512 * MiB" Remove this?
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 54: 51 Is 51 right (which would be 0xA2) or should this be 52 (which would be 0xA4, as shown in the table above?)
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 70: 0x4E As i said in the other comment, it seem a little odd to have this defined in the ASL file.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 73: #include <superio/fintek/f81803a/acpi/superio.asl> So it is in the SIO directory. Maybe delete the copy out of the mainboard then.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 35: TODO: Make ACPI use these values too. Either fix it or remove the TODO. Copying TODOs from one board to another is kind of bad form.
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/romstage.c:
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 4: * Copyright (C) 2015 Advanced Micro Devices, Inc. : * : What's the copyright on?
Maybe just remove everything and leave a blank file?