Richard Spiegel 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)
What I marked as resolved will be in the next commit, as soon as I send this. What is not marked as resolved will be uploaded on a second commit.
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
Will do.
https://review.coreboot.org/c/coreboot/+/33993/15//COMMIT_MSG@20 PS15, Line 20: simi;ar
similar
Oops...
https://review.coreboot.org/c/coreboot/+/33993/15//COMMIT_MSG@22 PS15, Line 22: Marc
Marc Jones
Thanks.
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.
Done
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?
Will do.
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. […]
Correct, will remove the help.
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. […]
Left over from Marc. It's hardcoded on the header file, and it's not 100... it's 0x220 (using an UART window in the LPC host controller). Will remove.
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 fi […]
Will do.
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. […]
Will do.
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. […]
They are, will revisit.
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?
Done
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. […]
Will check
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?
True, the whole file should be there. Will move.
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?
Done
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 […]
Done
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. […]
Ack
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?
Will have to verify. Will try.
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?
Done
https://review.coreboot.org/c/coreboot/+/33993/15/src/mainboard/amd/padmelon... PS15, Line 140: AzaliaCodecAlc286Table
Is this the correct codec for padmelon?
Not sure, copy from Marc's code. Will double check.
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?
Not sure, again, got from Marc. Possibly because of 3 different SOCs that can be populated to this board? Will check.
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. […]
They are needed for those who don't have a modified serial cable (connecting DCD to DTR and DSR, plus connecting RTS to CTS). When you buy it on any store, they don't have these modification (my case). Maybe I should add this explanation to the comment.
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?
Done
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 a […]
52 (0xA4). Done.
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.
Will check best way.
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.
Will do.
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.
Removed.
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? […]
This is a placeholder, if there ever is something platform specific to add to romstage. Maybe add a comment to that effect and remove everything else.